Carry engine and port in the server identity, re-keying nothing (#2218) - #2278
Conversation
The storage name was host + database + read-only intent only, so a SQL Server and a PostgreSQL instance on ONE host collided into a single server_id and interleaved their histories -- as did two PostgreSQL instances differing only by port, both of which #2213 made first-class configuration. The interesting constraint is what could not change. Lite derives server_id FRESH at runtime from the shared BuildStorageName, everywhere, with no stored-id fallback: GetServerNameForStorage hashes it on every read. So altering what that returns for an existing server re-keys it in Lite and orphans all its history, silently -- the same harm as #2158 from the other direction. That refutes simply adding the fields. So the parameters are OPTIONAL and append nothing at their defaults, keeping Lite's three-arg call byte-identical. Verified against a re-statement of the pre-change rule rather than hand-written strings, so it holds for any input. A SQL Server entry passing them is unchanged too, which is what lets Darling pass Engine and Port unconditionally: Engine folds to no token for SQL Server, and Port is PostgreSQL-only -- SQL Server carries a non-default port inside the host as host,1433 and is already discriminated there. Darling's registered PostgreSQL targets do not re-key either, for a different reason: their id comes from the store and is only ever derived for an entry with no row yet. That is why #2158 was a prerequisite for this and not a sibling. Every spelling folds to one token (postgres/PostgreSQL/pg, any casing), so a colleague's capitalisation cannot mint a second identity for one instance -- a split history nothing downstream could diagnose. An unrecognised engine appends nothing rather than being interpolated raw, so a typo cannot either. Suffix order is fixed (engine, port, :RO) so two callers with the same facts cannot produce two names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f62617d to
2b90381
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var trimmed = engine.Trim(); | ||
| if (trimmed.StartsWith("postgres", StringComparison.OrdinalIgnoreCase) | ||
| || trimmed.Equals("pg", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
EngineToken only recognizes strings starting with "postgres" or exactly "pg", but that's not the full set of engine spellings this codebase accepts as PostgreSQL. DarlingConfig.TargetEngine (DarlingConfig.cs:1162) and DarlingMcpServerAdminTools.ResolveEngine (DarlingMcpServerAdminTools.cs:574) both also accept "aurora" and "aurora-postgresql" — the latter's doc comment even says it "accepts the same aliases the parser does, so the two never disagree about a value they both accept," which is no longer true here.
Concretely: an entry with "engine": "aurora-postgresql" (or "aurora") resolves to CollectorTargetEngine.PostgreSql via TargetEngine and connects with the Postgres driver, but EngineToken returns null for it, so StorageName appends no :pg suffix. That's exactly the collision this PR sets out to fix (#2218) — a PostgreSQL instance sharing a host string with a SQL Server entry — reopened for the "aurora"/"aurora-postgresql" spellings. It also breaks the "every spelling folds to one identity" invariant: "postgres" and "aurora" used for the same logical engine family produce different discriminators.
The test suite doesn't cover this gap either — AnEngineThatIsNotPostgresAppendsNothing and EverySpellingOfPostgresFoldsToOneIdentity in ServerIdentityEngineAndPortTests.cs never exercise "aurora" or "aurora-postgresql".
Suggest mirroring the same accepted-spellings list used by TargetEngine/ResolveEngine here (or better, sharing one canonical list) rather than a StartsWith("postgres") heuristic.
|
Review summary: two findings, both about the same class of gap - call sites this PR left un-migrated to the new engine/port-aware identity. Finding 1 - Attempted as an inline comment, but GitHub rejected it since the file isn't in this PR's diff, so noting it here instead.
Suggest passing Finding 2 - Posted as an inline comment on Everything else in the diff (the optional-args defaulting, the byte-identical 3-arg Lite path, the fixed suffix ordering, the zero/negative-port handling) checked out against the stated invariants, and the reasoning for why Lite's call site is intentionally left unchanged (no stored-id fallback, so any change to what |
CI caught three tests holding their own copy of the identity rule, which stopped matching the moment the derivation grew engine and port. All three are legitimate expectation changes -- PostgreSQL identities intentionally changed -- not papering over: - PostgresEngineGateBehaviorTests hashed PG hosts with the 3-arg form while its MonitoredServer says Engine = "postgres", so it computed an id the product never uses and the gate returned "server not monitored" instead of the arm under test. Its helper now takes the engine, defaulted to null so the SQL Server call sites are untouched. - PostgresTargetConfigTests pinned the storage name for a PG server; ":pg" now sits between the database and ":RO". Extended to cover the port too, since that string IS the documentation of the rule. - ServerIdentityFromStoreTests hashed (host, database, readOnlyIntent) by hand for a server that carries Engine = "postgres" and Port = 6432. Now derived from the server's own StorageName, so it cannot drift again -- the old shape failed as "the seed wrote the wrong id" rather than "the test's copy of the rule is stale". Swept the rest of both suites: every other BuildStorageName caller uses a default engine and port, so their expectations are byte-identical and unaffected. SharedCollectorDefaultsPinTests' four pins are exactly the no-re-key cases and still hold, which is the guard proving the change is backwards compatible. Verified against the real helper: the two updated strings, that the seeded PG id genuinely differs from the stale hash (so the fix was required rather than cosmetic), the four no-re-key pins, and that sqlserver-default entries are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| private static string? EngineToken(string? engine) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(engine)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var trimmed = engine.Trim(); | ||
| if (trimmed.StartsWith("postgres", StringComparison.OrdinalIgnoreCase) | ||
| || trimmed.Equals("pg", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return "pg"; | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
Bug: EngineToken disagrees with the codebase's actual engine gate, and un-fixes #2218 for two documented engine spellings.
EngineToken only recognizes strings that start with postgres or equal pg. But that's narrower than the parsing rule used everywhere else in this PR's own codebase:
MonitoredServer.TargetEngine(Darling/PerformanceMonitor.Darling.Service/DarlingConfig.cs:1160-1164) treats"aurora-postgresql"and"aurora"as PostgreSQL.- The MCP
add_serversgateResolveEngine(DarlingMcpServerAdminTools.cs:571-578) accepts the same aliases and documents them in its own error message:"also accepted: \"postgresql\", \"pg\", \"aurora\", \"aurora-postgresql\"". StoreConfigProvider.cs:426-429deliberately persists the raw operator string rather than the parsed enum, specifically "so ... the single parse inMonitoredServer.TargetEnginestays the only place that interprets it." This PR adds a second interpreter here that disagrees with the first.
Concrete failure: register a fresh Darling server with "engine": "aurora" (or "aurora-postgresql") on a host that already has a SQL Server target registered. TargetEngine correctly routes it to the Postgres collector path, but EngineToken("aurora") returns null, so StorageName gets no :pg suffix — it collides with the SQL Server entry's storage name and server_id. That's the exact #2218 collision this PR exists to fix, reproduced for two engine spellings the rest of the codebase explicitly supports.
The new test file (Lite.Tests/ServerIdentityEngineAndPortTests.cs) only exercises postgres/postgresql/pg/Postgres/etc. — no aurora or aurora-postgresql case — which is presumably why this wasn't caught.
Suggest having EngineToken delegate to (or share a table with) the same canonical alias list TargetEngine/ResolveEngine already use, rather than re-implementing engine recognition with its own, narrower rule.
| if (port > 0) | ||
| { | ||
| name += ":" + port.ToString(System.Globalization.CultureInfo.InvariantCulture); | ||
| } |
There was a problem hiding this comment.
Minor, same family as the EngineToken issue above: this appends :{port} whenever port > 0, with no check that the target is actually PostgreSQL — but the doc comment on BuildStorageName (and MonitoredServer.StorageName in DarlingConfig.cs) both describe port as inert for a SQL Server entry, "because SQL Server carries a non-default port inside Host."
That's true by convention, not by anything enforced: DarlingConfig.Validate() (DarlingConfig.cs:299) only range-checks Port, never requiring it to be 0 when TargetEngine != PostgreSql, and the MCP ResolvePort gate (DarlingMcpServerAdminTools.cs:586-605) doesn't check engine either. So a SQL Server registration that happens to carry a nonzero port (e.g. a copy-pasted add_servers payload) silently gets a different server_id than an otherwise-identical one that doesn't — an avoidable discriminator leak for the SKU this change is supposed to leave untouched.
ReviewReviewed the identity-derivation change ( One correctness bug found, left as inline comments:
Everything else — the suffix ordering, the zero/negative port handling, the No SQL Server missing-index DMV concerns (none of this touches T-SQL), no secrets/injection issues spotted. |
DocCommentHygieneTests caught it: inserting a doc comment above ServerIdFor pushed the block that was already there into a stack, so XML docs took the last one and only a human reading the file would have seen two. MERGED rather than deleted, per the test's own warning. The original point -- derive through the same helper the worker uses, so the test cannot drift from the lookup -- is the reason the helper exists at all, and #2218 is an instance OF that drift rather than a replacement for it. Deleting the first block would have lost the general rule and kept only the example. The detector I have for this only helps if it runs on the files a commit actually changed, not the ones I remember touching -- it is now driven off git diff --name-only, which reports clean across all six files here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| var trimmed = engine.Trim(); | ||
| if (trimmed.StartsWith("postgres", StringComparison.OrdinalIgnoreCase) | ||
| || trimmed.Equals("pg", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return "pg"; | ||
| } |
There was a problem hiding this comment.
EngineToken only recognizes postgres* and pg, but the codebase treats aurora and aurora-postgresql as equally valid Postgres spellings elsewhere:
MonitoredServer.Engine's own doc comment: "accepted spellings: postgres, postgresql, pg, aurora-postgresql" (DarlingConfig.cs:1073)MonitoredServer.TargetEngine:"postgres" or "postgresql" or "pg" or "aurora-postgresql" or "aurora" => CollectorTargetEngine.PostgreSql(DarlingConfig.cs:1162)DarlingMcpServerAdminTools.ResolveEnginenormalizes the same five spellings (Mcp/DarlingMcpServerAdminTools.cs:574)StoreConfigProvider.cs:427's comment even names"aurora"explicitly as an alias the store round-trips verbatim from a hand-editeddarling.json
"aurora" doesn't start with "postgres" and isn't "pg", and "aurora-postgresql" starts with "aurora" not "postgres" — so both fall through to return null here, same as an unrecognized/typo'd engine.
Concretely: an operator who writes "engine": "aurora" (a documented, tested-elsewhere spelling — see TargetEngine's test coverage in PostgresTargetConfigTests.RecognizesThePostgresSpellings) gets a server whose TargetEngine correctly resolves to PostgreSQL (connects via the pg driver, runs pg detection queries, etc.) but whose StorageName gets no :pg suffix — so it can still collide with a SQL Server target on the same host, which is exactly the #2218 defect this PR sets out to fix, just for this one accepted spelling. Lite.Tests/ServerIdentityEngineAndPortTests.cs covers postgres/PostgreSQL/pg/casing variants but never exercises aurora or aurora-postgresql, which is presumably how this gap got through.
Suggest folding through the same recognizer TargetEngine uses (or delegating to it) so there's one accepted-spellings list instead of two that can drift:
var trimmed = engine.Trim();
if (trimmed.StartsWith("postgres", StringComparison.OrdinalIgnoreCase)
|| trimmed.StartsWith("aurora", StringComparison.OrdinalIgnoreCase)
|| trimmed.Equals("pg", StringComparison.OrdinalIgnoreCase))
{
return "pg";
}
|
Reviewed. One correctness bug found, posted inline on
Everything else looks solid:
|
1 participant
Closes #2218.
The defect
server_idcame from host, database and read-only intent only — carrying neither the engine nor the port, both of which #2213 made first-class configuration. So a SQL Server and a PostgreSQL instance on one host collided into a single identity and interleaved their histories, and so did two PostgreSQL instances distinguished only by port.The interesting part is what could not change
Lite derives
server_idfresh at runtime from the sharedServerIdHelper.BuildStorageName— everywhere, on every read, viaRemoteCollectorService.GetServerNameForStorage— and has no stored-id fallback the way Darling does.So simply adding the fields is refuted by its consequences: it would re-key every Lite server and orphan all of its collected history, silently. That's the same class of harm as #2158, arrived at from the other direction.
Hence the new parameters are optional and append nothing at their defaults, which keeps Lite's three-argument call byte-identical.
Why nothing re-keys, in three parts
Enginefolds to no token;Portis PostgreSQL-only and stays 0, because SQL Server carries a non-default port inside the host ashost,1433and is already discriminated thereStoredServerId) and is only ever derived for an entry with no row yetThat last row is why #2158 was a prerequisite for this, not a sibling: with identity assigned rather than re-derived, changing the derivation can only affect what a fresh registration gets. It's also what lets
DarlingConfig.StorageNamepassEngineandPortunconditionally instead of branching — the defaults are the backwards-compatible case.Two ways this could still have split a history
postgres,PostgreSQL,pg, any casing, and surrounding whitespace all fold to one token. A colleague writing"PostgreSQL"where another wrote"postgres"must not get a second identity for one instance — that's a split history caused by capitalisation, which nothing downstream could diagnose.Suffix order is fixed (engine, then port, then
:RO) so two callers supplying the same facts can't produce two names, and:ROstays last so the existing convention survives.Testing
The tests live in Lite.Tests deliberately: Lite is the SKU with no stored-id safety net, so it's the one whose invariant needs guarding.
The no-re-key property is asserted against a re-statement of the pre-change rule rather than hand-written expected strings, so it holds for any input rather than only the handful someone thought to list — and it's asserted on the derived
server_id, not just the string, since that's what stored rows are keyed by.I also compiled the real helper into a
net10.0harness and executed the whole matrix locally — 40/40, including the numericserver_idequality for every legacy form, both collision fixes, all eight engine spellings, the fixed suffix order, and zero/negative ports. Sample:Full solution builds, 0 warnings; doc-comment hygiene clean on all three files.
Note for #2228
This does not address the remaining half of #2228 (refusing a new registration whose (host, actual database) pair collides with an existing one), and can't: those two registrations genuinely differ in config, so no derivation change can tell they resolve to one database. #2277's connect-time tripwire is what covers that ground.
🤖 Generated with Claude Code