Repository navigation
An edit keeps its server identity, so history stays attached (#2158) - #2276
Conversation
The Add/Edit save re-derived server_id from host/database/read-only-intent on every save, so fixing a hostname typo wrote a row under a new id and deleted the old one. The registry stayed tidy and every collect.* row keyed to the old id was orphaned with nothing pointing at it -- which is why nobody noticed: the server just reads as though it was never monitored. An edit now keeps the row's identity and rewrites the address in place. Derivation runs only on Add, where there is no history to lose. Assigned rather than derived, argued from consequences: #2158 says a config edit must not change the identity; #2228 says two configs resolving to one real database must not be two identities, which no config-derived hash can decide because the configs genuinely differ; #2218 says two instances on one host need distinguishing, which wants MORE fields in the derivation and makes #2158 strictly worse. One shape satisfies all three -- allocate once, never recompute, treat the derived address as a lookup key, and get what the target actually IS from the connection. StoredServerId and the connector's config.ServerId already did their half; the Viewer's save was the last site. Two things the fix had to carry. The collision guard compared a DERIVED id against the registry, which only holds while every row's id equals the hash of its own address -- left alone, an edit could point a second registration at an address another server already monitors and the guard would never fire, because the id it looked up belongs to nobody. It now matches the address columns (IS NOT DISTINCT FROM for the nullable database; = never matches NULL, so every server-scoped registration would read as "free") and compares ids afterwards, which still lets a rename or re-credential through. And the darling.json reconcile matched on id alone, so after an edit it would call a monitored server absent and advise re-adding it -- wrong advice every start about the server just fixed. Now id OR name, either-or so a same-named sibling cannot hide a genuinely unmonitored one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var occupant = await _dataService.GetMonitoredServerByAddressAsync(row.Host, row.Database, row.ReadOnlyIntent); | ||
| if (occupant is not null && occupant.ServerId != row.ServerId) | ||
| { | ||
| StatusText.Text = "A server with this address (and database / read-only intent) is already monitored. Edit it from Manage Servers instead."; | ||
| SaveButton.IsEnabled = true; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Correctness: the new guard only checks address collisions, and the id-collision check it replaced is gone — so Add can now silently steal another server's identity/history.
ComputeServerId is a pure deterministic hash of (host, database, readOnlyIntent) (ViewerDataService.MonitoredServers.cs:251), and config_monitored_servers.server_id is the only unique/primary key on the table — there's no UNIQUE(host, database, read_only_intent) constraint (see migration-ladder-v3.3.0.sql). Before this PR, the guard was:
if (_originalServerId != row.ServerId
&& await _dataService.GetMonitoredServerAsync(row.ServerId) is not null)which caught any pre-existing row already sitting on the id row.ServerId — including on Add, since _originalServerId is null there.
The new guard only asks "does anything currently sit at this address":
var occupant = await _dataService.GetMonitoredServerByAddressAsync(row.Host, row.Database, row.ReadOnlyIntent);
if (occupant is not null && occupant.ServerId != row.ServerId)Walk through this sequence, which is exactly the scenario #2158 exists to fix:
- Server A is Added at
(h1, d1, false)→ gets idX = ComputeServerId(h1, d1, false). - Server A is later Edited to
h2(per this PR, it keeps idX, correctly). h1gets reused for a new instance (IP/hostname reuse is routine in cloud/ephemeral infra). Operator Adds it as a brand-new server with hosth1.row.ServerIdderives toXagain — the same id A now owns under a different address.GetMonitoredServerByAddressAsync(h1, ...)finds no occupant (A lives ath2now), so the guard passes.UpsertMonitoredServerAsyncrunsINSERT ... ON CONFLICT (server_id) DO UPDATE SET name=EXCLUDED.name, host=EXCLUDED.host, ...— every column, including secrets — silently overwriting A's row with the new server's data. A'scollect.*history is now misattributed to the new Add, with no error, no log line, nothing to alert the operator.
This is the same failure class this PR fixes for Edit, reopened for Add. Since Add always writes a freshly-derived id with no _originalServerId, it seems like the id-existence check should be kept (e.g. _originalServerId != row.ServerId && await _dataService.GetMonitoredServerAsync(row.ServerId) is not null) in addition to the new address-based check — the address check alone doesn't answer "does this id already belong to someone else."
Review summaryScope: this PR is entirely C#/Postgres-store code in the Darling app (no T-SQL, no Lite files touched), so there's no Lite/Darling parity concern here — Lite has no equivalent multi-server registry/Add-Edit dialog. The core fix (identity survives an edit, derivation only on Add, collision guard rechecked against address instead of a derived id, reconcile matches id-or-name) is sound and well-tested for the Edit path. Left one inline finding on a gap it opens on the Add path:
Minor, non-blocking: the Everything else checked out: parameterized SQL throughout (no injection surface), |
1 participant
Closes #2158.
The defect
The Add/Edit save re-derived
server_idfrom host/database/read-only-intent on every save — the code said so as its intent:So an operator fixing a hostname typo wrote a row under a new id, and the old row was deleted. The registry stayed tidy and every
collect.*row keyed to the old id was orphaned with nothing pointing at it.That's why it went unnoticed: nothing looks broken. The server simply reads as though it had never been monitored.
An edit now keeps the row's identity and rewrites the address in place. Derivation runs only on Add, where there is no history to lose.
Why identity must be assigned rather than derived
Not a preference — it falls out of the consequences. Three issues pull on this identity and they pull in opposite directions:
So "add engine and port to the hash" is refuted by its own consequences: it fixes #2218 and makes #2158 strictly worse, while doing nothing for #2228. One shape satisfies all three — identity allocated once and never recomputed, the derived address demoted to a lookup key, and what the target actually IS taken from the connection.
Most of that was already built and pointed this way:
StoredServerIdmade the store authoritative,DarlingServerConnectorreadsconfig.ServerIdrather than re-hashing, andServersOnlyInFileeven carried a comment predicting this transition. The Viewer's save was the last place identity was recomputed.This also unblocks #2218 as a consequence rather than a prerequisite: once derivation only runs at Add, adding engine/port to it is harmless, because it can no longer orphan anything.
Two things the fix had to carry with it
The collision guard. It compared a derived id against the registry, which only works while every row's id equals the hash of its own address — exactly the invariant this change gives up. Left as-is, an edit could point a second registration at an address another server already monitors and the guard would never fire, because the id it looked up belongs to nobody. That's #2228's shape reached from the registry side. It now matches the address columns and compares ids afterwards, which is what still lets a rename or a credential change through as "collided with myself."
IS NOT DISTINCT FROMfor the nullabledatabaseis load-bearing:=never matches NULL, so every server-scoped registration — the common case — would read as "address free" and the guard would pass for all of them.The darling.json reconcile. It matched on id alone, so after an edit it would report a server that is monitored as absent — and that log line gives advice ("add them with the Viewer's Add Server dialog"), so being wrong is worse than being silent: it would tell the operator to re-add the one server they'd just fixed, on every start. Now id or name; either-or rather than name-only, because nothing enforces display-name uniqueness and name-only would let a same-named sibling hide a genuinely unmonitored server.
Testing
The save path is a WPF dialog, so the identity-preservation and guard-rewrite halves are pinned at the source — a re-derivation there is invisible in every other test and silently discards history, which is worth holding at the only level available. The pins assert both the new shapes and the absence of the old ones (the derived-id guard, the delete-old-identity step), so a partial revert fails.
ServersOnlyInFileis pure, so its six cases are ordinary tests: an edited server not reported, the same server reported without the name arm (so the test can't pass vacuously), a deleted server still reported, an id match alone sufficing, neither-present reported, and the no-names-supplied overload staying id-only so an un-updated caller can't silently suppress the warning.I executed the predicate's logic and verified all the source pins locally — 20/20, including doc-comment hygiene on all four files. Full solution builds, 0 warnings.
Not in this PR
DB_NAME(), orcurrent_database()+inet_server_port()on PG) and refusing when two registrations report the same real instance. That's the only thing that closes [BUG] N registrations silently resolving to one database get N identities and N copies of every incident: no DB_NAME() cross-check exists #2228, and it's a connect-path change rather than a registry one.🤖 Generated with Claude Code