Repository navigation
Editing a registered server's settings in darling.json is silently ignored — only ADDING a server is warned about #2552
Description
Found while standing up a live PostgreSQL target against a container build of dev.
What happens
WarnAboutFileOnlyServersAsync reads SELECT server_id, name FROM config_monitored_servers and compares names and ids only. If every server in darling.json is already registered, fileOnly.Count == 0 and it returns immediately. Nothing anywhere compares the file's per-server settings against the store's — grep for a property-drift comparison in StoreConfigProvider.cs finds none.
So for a server that is already registered, every setting in darling.json is dead text: trustServerCertificate, username, password, port, database, encryptMode, excludedDatabases, capturePlans, displayName. The file is read, parsed, validated, logged as Loaded configuration from /config/darling.json: 1 server(s) — and then the store's row is used instead, with no indication that the two disagree.
Why this is worse than it sounds
#2252/#2254 already warn — well — about the adjacent case:
darling.json lists N server(s) that are NOT monitored and never have been … The store is authoritative after the first seed, so adding a server to the file does not register it and a restart cannot change that
An operator who reads that learns "adding via the file does not work." The natural inference is that the file does still drive the servers it already knows about. It does not, and nothing corrects that inference.
The web.network / mcp.network blocks each log a full paragraph explaining they are file-only and restart-only and that the control plane owns the rest. The server list — far more likely to be edited than either — says nothing.
How I hit it, which is the realistic path
- Added a PostgreSQL target. First connect failed:
[pgtarget] Connect failed, retrying in 60s: SSL connection requested. No SSL enabled connection from this host is configured.Good message — names the target and the cause. - Enabled TLS on the target. Now:
Exception while performing SSL handshake(self-signed cert, soSslMode.VerifyFullcorrectly refuses). - Applied the documented fix — set
"trustServerCertificate": trueindarling.json— and restarted. - Byte-identical error. No new information, no hint the edit was not read.
The store row explained it:
name | trust_server_certificate | created_at | modified_at
pgtarget | f | 2026-08-22 22:27:15.069708 | 2026-08-22 22:27:15.069708
created_at == modified_at, still f. The edit never left the file.
That is a bad loop to be stuck in: a connection failure is exactly the kind of problem an operator fixes by editing config and restarting, and this is the one class of edit that produces an unchanged error with no explanation.
Worth deciding, not assumed
I am not proposing that the file start overriding the store — store-authoritative is a deliberate design and #2254 is explicit about it. The gap is that the disagreement is invisible. The minimum fix is to extend the existing warning to cover servers present in both, comparing the settings that actually affect behaviour and naming the ones that differ, in the same voice as the existing message: what the file says, what the store says, and that the store wins and how to change it (Viewer's Add/Edit Server, or MCP add_servers).
Two things to settle first:
- Which fields count. Comparing everything invites noise from cosmetic normalization —
encrypt_modeis already normalized on seed (StoreConfigProvider.cs:350mirrors the normalizer precisely so a casing choice cannot fail the seed), so a naive comparison would warn on a difference that does not exist. The comparison has to run against the same normalizer. - Passwords must not be compared or printed. The file may carry a
file:/env:reference while the store carries a DPAPI blob; those are not comparable and the answer is almost certainly to exclude the credential from the drift check entirely rather than to try.
A --test-connection caveat probably belongs here too: per the #2252 message it already reads darling.json and reports PASS for servers that collect nothing. The same is true for settings — it would test the file's trustServerCertificate, not the store's, so it can report PASS for a configuration the service will never use.
Activity
erikdarlingdata commentedon Aug 23, 2026
Fix is up in #2556 (targets
dev, not merged). The report holds — the mechanism, the reproduction and the "the #2254 warning makes this worse" reading are all confirmed against the source. Three corrections to the framing, one of which changes what the message can say.1.
capturePlansis not a per-server setting.MonitoredServerhas no such property;capturePlansis a top-level service setting seeded intoconfig_service.config_monitored_servers.capture_plansdoes exist, but the seed writes itNULLunconditionally, so there is no file value to disagree with it. Every other field in the list is a real per-server setting and every one of them was dead text.2.
add_serverscannot be part of the remedy. This is the one that changes the message.add_serversrefuses an already-registered server:PartitionDuplicatesskips it asstatus:"duplicate"before it validates anything. So "the Viewer's Add/Edit Server dialog, or MCPadd_servers" would have sent an operator to a tool that silently does nothing — the exact failure the warning exists to end. There is no MCP edit verb at all;remove_server+add_serversis the only headless path and it is a heavier hammer than a startup warning should recommend. The line names the Viewer's Manage Servers window and rulesadd_serversout explicitly, echoing the Viewer's own duplicate message ("Edit it from Manage Servers instead").3.
StoreConfigProvider.cs:350is the plan-XML normalizer, not theencrypt_modeone. That line isNormalizePlanXmlCompression, mirroring V62's case-sensitive CHECK onplan_xml_compression.encrypt_modehas no CHECK constraint and the seed writes it raw (line 512). So the hazard is real but it lives at the connect path, not the seed:MonitoredServerConnectionfolds the mode withTrim().ToUpperInvariant()and fails closed to Mandatory, which is why"strict"vs"Strict"— and a typo vs"Mandatory"— build the same connection. The comparison folds through that switch rather than through a seed-side normalizer.The audit you asked for turned up five more folds of the same kind, not just
encryptMode:databaseis the engine's implicit default, so a NULL column against an explicit"master"(or"postgres") is one connection;engineis stored raw by design (V68), so"aurora"and"postgres"routinely sit on opposite sides of one identity — folded throughTargetEngine;port0means the driver default, i.e.5432;excludedDatabasesis spliced as aNOT INset byDatabaseExclusionFilter, so order, repetition, case and whitespace change nothing;monthlyCostUsdis adecimal, which carries its scale, so1200and1200.00render differently and are one figure.Three fields are also gated rather than compared, because they are inert on the target they sit on:
readOnlyIntent/multiSubnetFailoverare SqlClient concepts the Npgsql builder never sees,portis not a SQL Server concept (it rides in the host ashost,1433), andusernameonly reaches the connection string under SQL auth. The gates read the store's engine and auth, since the store is what the service connects with; a disagreement about the engine itself gets its own line.On the credential: excluded entirely, and structurally rather than by intention.
encrypted_passwordis not in theSELECTlist, so there is no blob in memory for a later edit to leak. And it must not be compared even if it were read — a file entry carrying anenv:/file:reference or a dev plaintext password against a store row carrying a DPAPI blob is the supported shape thatBuildServerFromRow's read-time backfill exists to serve, so comparing them would report a working configuration as drift on every start. Proven by reverting: adding the comparison puts anenv:reference and a DPAPI blob straight into the rendered warning.On the
--test-connectioncaveat: yes, but conditionally. It is appended only when a connection-relevant field drifted. The verb probes the file's connection settings, so it can report PASS for a connection the service will never make — which is your reproduction exactly. But it does not exercise the display name, the exclusion list, the cost figure or the delivery override, so appending it there would be claiming more than the warning can support.Also picked up your point about the server list saying nothing:
darling.sample.json'sserversblock now carries the seed-once paragraph theweb.network/mcp.networkblocks have always had, including which two things in that block are still read every start (the SQL-auth secret, and the whole file when the store is unreachable).The store still wins. Nothing about store-authority changed.
A paused server gets its own sentence, and the covered field set is d…
Merge pull request #2556 from erikdarlingdata/fix/2552-server-setting…
erikdarlingdata commentedon Aug 23, 2026
Fixed in #2556 (merged to
dev), and verified live — the one thing that PR listed as unverified.I reproduced the original scenario on a running service against a real seeded store: took a registered PostgreSQL server and edited four of its settings in
darling.jsonso the file disagreed with the registry ondatabase,username,trustServerCertificateandport. On restart, the service now says so:Every drifted field named, both values shown, the remedy correct, and the
--test-connectioncaveat present. That exercises the liveSELECTshape, theMonitoredServerhydration from the reader, and the log line as the service's own logger renders it — none of which CI's fixtures cover.Three corrections to this issue, all mine:
capturePlansis not a per-server setting.MonitoredServerhas no such property — it is top-level, and the store's per-server column seedsNULL. I listed it as silently-ignored; it was never file-driven per server.add_serverscannot be the remedy, and recommending it would have reproduced this very bug.PartitionDuplicatesskips an already-monitored server asduplicatebefore validating anything, so an operator following that advice would have been sent somewhere that silently does nothing. There is no MCP edit verb at all. The shipped message names the Viewer's Manage Servers window and rulesadd_serversout explicitly.StoreConfigProvider.cs:350isNormalizePlanXmlCompression, not the encrypt-mode normalizer I claimed.encrypt_modehas no CHECK constraint and the seed writes it raw. The normalization hazard I described is real, but it lives at the connect path — so the comparison folds throughMonitoredServerConnection's own fail-closed switch rather than a seed-side normalizer.The comparison turned out to need more care than I credited: four fields are engine-asymmetric —
Strict≡Mandatoryon PostgreSQL only, blankdatabasefolds to an engine-specific default, PG port0→5432, anddatabase/excludedDatabasesare case-sensitive on PostgreSQL but not SQL Server. Inert fields are not compared at all, gated on the store's engine and auth mode.Credentials are excluded structurally rather than by a rule someone could later relax:
encrypted_passwordis simply not in theSELECTlist, so no blob exists in memory to leak.One review finding is worth recording because the obvious fix was wrong: filtering disabled servers out of the store read would have caused #2158, since that same read feeds
ServersOnlyInFile— a paused server would then be reported as never-monitored. Disabled servers get their own Information line instead.Report when darling.json disagrees with a REGISTERED server's registr…
Let the drift exclusion cover the remediation credential, and say wha…
Add the remediation credential seam, the journal's actor and the evic…