Release 3.6.0 - #2669
Merged
Merged
Release 3.6.0#2669
Conversation
Give a PostgreSQL target PostgreSQL tabs on the web (#2530)
…er (#2535) Lite's CorrelatedTimelineLanesControl is a third copy of the five-separate- WpfPlot Overview surface that carried #2533 on the Darling viewer: SyncXAxes matched the five lanes' X-axis LIMITS but never gave them identical pixel geometry, so a lane whose Y ticks read six digits still starts its data area further right than one reading a fraction. Confirmed present by reading the shipped source, not assumed from the shared XAML. The fix already existed in the shared PerformanceMonitor.Ui.LaneAxisAligner that Lite already references, so this is the one call site #2534 filed as #2535: SyncXAxes now splits into set-limits / align / refresh, calling LaneAxisAligner.AlignLeftGutters between the two loops so every lane's X and Y limits are final before it measures. It has to run on every refresh, not once at Initialize, because ClearChart() -> WpfPlot.Reset() swaps in a fresh Plot and takes any axis floor with it. A source-parse wiring pin in Lite.Tests (not Darling.Tests, so a Lite-only edit fires the CI filter the guard needs) asserts the control calls the aligner inside SyncXAxes and before the refresh loop, bounded by brace matching so Lite's AddGhostLine helper - which sits a few members later and also calls Refresh() - can't satisfy it by accident. Confirmed red against the pre-fix source with a throwaway net10.0 harness running the same parse logic, since the xUnit suite itself only executes in CI. The behavioral coverage (the gutter is independent of plot width, and non-decreasing in plot height) is not duplicated here - those are properties of the shared helper, already pinned by Darling.Tests/LaneAxisAlignerTests.cs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…#2548) `get_pg_top_queries` put `queryid` on the wire as a JSON number. PostgreSQL's `queryid` is a signed int8 derived from a hash of the post-parse-analysis tree, so its values are spread over the whole 64-bit range and most sit past 2^53 -- where every parser that decodes JSON numbers as IEEE-754 doubles (`JSON.parse`, `json.loads`, most agent tooling) silently rounds. Reproduced against real `JSON.parse`: `-4185925123159566327` comes back `-4185925123159566300`. The value was never wrong on the wire; it was unrecoverable after parsing, which for an identity is the same thing. `queryid` is the ONLY identity a PostgreSQL statement has, and every use of it -- `WHERE queryid = ...`, matching a row on our screen to one on the instance, quoting it in a ticket -- is an equality join. A rounded metric is still approximately true; a rounded key matches nothing. Breaking response-shape change, taken deliberately. `get_pg_blocking`'s `root_backend_id` had the same defect in a WORSE form, and is fixed here rather than filed for later. The collector builds it by concatenating the backend's start epoch with its zero-padded pid, so every value is a 17-digit integer around 1.79e16 -- about 2x past 2^53, where adjacent doubles are 2 apart. Half of all backend ids are odd and have no representation at all, and an unrepresentable one does not round to nothing: it rounds onto its even NEIGHBOUR, which is a different backend. Measured over 200 adjacent pids, 100 lost precision and 99 landed on an id belonging to another backend. That is the field the tool's own description tells a reader to prefer for comparing a root blocker across captures. Scoped to those two. `database_id` / `user_id` are PostgreSQL oids (unsigned 32-bit, structurally unable to reach the range), `root_pid` is an int, SQL Server's `query_hash` / `plan_hash` already reach this surface as `0x...` text, and Query Store's `query_id` / `plan_id` are sequential bigint IDENTITY columns nowhere near 2^53. None has a defect to fix; none was touched. Both response bodies are split into `BuildTopQueriesJson` / `BuildBlockingChainsJson` so the wire shape can be asserted against the SHIPPED serializer instead of a re-implementation that would keep passing as the code drifted. `PgInt64IdentityWireShapeTests` pins both fields as strings, pins `database_id` and `root_pid` as NUMBERS so "stringify everything" cannot pass, asserts its own fixture is genuinely out of double range, and checks that four distinct backends stay four distinct ids after a double-decoding parse. Proven red by reverting only the two `.ToString(...)` calls: 15 checks fail. The web needs no change -- the Query ID column already rendered the raw value, so it simply starts showing the true digits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er-alignment Give Lite's Overview lanes the same shared gutter as the Darling viewer (#2535)
The WPF viewer cannot reference the service, so the PostgreSQL reads it needs for #2530 had to be reachable from somewhere both front ends see. They move to PerformanceMonitor.Darling.Storage, which the service, the viewer and the tests all already reference. Namespace change only: no query text, no ordinal mapping and no signature is touched, and every existing reader test still pins the same constants. Copying them into the viewer instead would have meant a second copy of, among others, a 200-line recursive blocking walk whose revisit guard, root attribution and truncation flag were each a separate review finding. Two copies of a query like that diverge, and the copy that diverges is never the one being read - the same reasoning CollectorEngineCapability's own comment gives for keeping one copy of a sentence both SKUs print. Storage gains InternalsVisibleTo("Darling.Tests") because the ordinal pins reach the internal Map*Row helpers, which followed the readers; the service keeps its own for everything that stayed behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`git grep IsPostgres` across the viewer returned nothing, so an Aurora target opened on the SQL Server correlated-lane Overview and got the whole nineteen-tab strip - tempdb, Trace Flags, Query Store, Plan Viewer, Always On - nearly all of it permanently empty and several tabs meaningless for the engine. It now gets six: Overview, Activity, Vacuum, Waits, I/O and Replication, the same six #2547 chose for the web, with the same ids and the same grouping so the two front ends do not teach one engine two shapes. Placement is DERIVED from CollectorCatalog in both directions rather than enumerated. All nine PostgreSQL collectors reach exactly one tab; a tenth turns the pin red naming itself, which is the check that did not exist while eight of them shipped MCP-only for three releases. The ViewerCollectorCoverageTests allow-list, which carried all nine as tracked debt, is now empty - and its reader-layer scan now follows the shared store readers the viewer NAMES, one hop outwards, so coverage still means "the viewer reads it" without demanding a duplicate copy of the SQL to satisfy a text match. Both Aurora-only panels are SHOWN on stock PostgreSQL, not hidden. Each prints CollectorEngineCapability.NotCollectedMessage - the same sentence the MCP surface and the web print, naming the server, the engine, the collector and the exact aurora_stat_* surface, ending "and never will". The defect here is unexplained emptiness, not emptiness; hiding would also make the tab strip a different shape on two PostgreSQL servers in one fleet. For the same reason the Overview grid is built from the CATALOG rather than from collection_log: a gated-off collector writes no log row at all, so a log-driven grid would drop precisely the row an operator most needs explained. Mechanics worth knowing: - The six TabItems CONTINUE the same TabControl at indices 19-24 rather than living in a second one. For a PostgreSQL server the nineteen SQL Server tabs are collapsed and these six shown, so both sets keep fixed indices and every drill-down that navigates by an index constant is untouched. - Each PostgreSQL index gets its OWN dispatch arm. Falling through to `default:` would run the SQL Server overview lanes - four collectors that cannot run on this engine - at a PostgreSQL server. - engine_kind and sql_engine_edition are selected by BOTH server reads. The sidebar uses ManagedServersSql on any seeded store, i.e. every real deployment, so a discriminator on only ServersSql would have left every PostgreSQL target on the SQL Server tabs while a unit test passed. - Only a POSITIVE claim switches. A null or unrecognised token keeps the SQL Server tabs and shows no engine badge at all, because the tabs such a server gets are a default rather than a finding. - The display projections keep the store's sentinels out of the cells: -1 is not a number, an untracked pg_stat_io write counter is not 0, a NULL recurrence is "cannot tell" and not "once", temp blocks are not bytes, and every timestamp goes through ViewerTimeHelper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ility gate (#2554) Two defects found by driving all nine get_pg_* reads against a live PostgreSQL 16 target. Eight answered; this one returned `42702: column reference "queryid" is ambiguous`. **Defect 1 — the SQL did not parse, on any engine.** #2219 added `LEFT JOIN collect.pg_statement_text AS t` so the read could carry statement text. That put `t.queryid` in scope beside `differenced.queryid` and made the unqualified references ambiguous. 42702 is a PARSE-time error, so the read threw on every call from #2219 onward — on Aurora, the one engine it exists for, as much as anywhere — and a store with zero rows failed exactly the way a full one did. The one-word fix in the report does NOT work, and that was measured rather than assumed: qualifying only the `GROUP BY` still fails on the select-list `queryid`, which is the reference PostgreSQL names first. Both are qualified. **Nothing caught it because nothing executed it.** About a dozen tests assert things about this query's TEXT and all of them passed throughout; a substring assertion cannot resolve a name, only a server can. So the deliverable is not the qualifier but `DarlingPgReadSqlParsesLiveTests`, which PREPAREs EVERY shipped PostgreSQL read against a real PostgreSQL. PREPARE runs the whole front end and stops before execution, so it needs no fixture and no rows — which is exactly why it catches a defect that zero rows was never a defence against. The reads are discovered by reflection, not listed, so a new one is covered the day it lands; the discovered COUNT is asserted too, so a filter that stopped matching cannot turn the guard into a test that passes by finding no work. All 12 shipped read constants across the 9 readers now parse. **Defect 2 — the capability gate was downstream of the throw.** It sat inside `if (rows.Count == 0)`, so it only spoke when the query SUCCEEDED and returned nothing. On stock PostgreSQL, where pg_statement_stats can never run at all, the parse error surfaced as a raw SQL error where the sibling get_pg_wait_stats gives "does not run on that engine ... and never will". The gate is now consulted on the throw path. Deliberately NOT moved ahead of the read. DarlingEngineCapability's contract is explicit that every call site asks AFTER its read came back empty, so a server whose registry row says one engine while its collected rows say another still gets its DATA rather than an explanation of why it cannot have any. Asking first trades this defect for that one across every read; on the throw path there is no data to prefer, so the same reasoning points the other way. Guarded without being vacuous. Once defect 1 is fixed the read no longer throws, so an assertion on the ordinary empty path would pass with or without the gate fix. The throw is therefore INDUCED — delta_calls at bigint extremes makes the read's `CAST(SUM(...) AS bigint)` overflow at runtime, deterministic and with no DDL against the shared store — and the discriminating half is the Aurora case: the same fault on an engine that CAN collect must still read as an error, or the fix would just be an exception swallower. Sibling sweep: all nine PostgreSQL tools have exactly one catch block and only this one consults the gate there, but with defect 1 fixed no other read can throw, so the other eight are recorded rather than changed. Three text pins that asserted the bare `GROUP BY queryid, database_id` are updated to the qualified form, and say which form is correct rather than merely that a GROUP BY exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All three claude[bot] findings were real. Two of them are the same defect - a field the shared reader goes to the trouble of returning that no viewer column shows - so the fix is an invariant rather than two columns, per the rule that the second instance of something is a sign you are fixing the wrong level. EveryFieldTheSharedReadersReturn_ReachesAColumn_OrIsFoldedWithAReason reflects over the ten reader row records against their display classes: every field must reach a column, or appear in FoldedFields naming the column that carries the fact and why that shape reads better. Derived both ways - a new reader field turns it red until something happens, and an exemption for a field that no longer exists is deleted the moment the reader stops returning it. Proved red by deleting exactly the two columns review found, each naming itself. It found three more on its first run, which is the point: PctTowardMultixactWraparound had no column at all (the multixact side has its own shutdown ceiling and a subtransaction-heavy workload reaches it first, with nothing on the XID columns to say so), and the two freeze_max_age settings were renamed rather than shown. The findings themselves: - Replication gains a Conflicting column, folded into the invalidated row highlight. A logical slot whose needed rows were vacuumed away by a recovery conflict is a different failure from invalidation-by-WAL-size, with a different cause (hot_standby_feedback, not max_slot_wal_keep_size), so it is its own column rather than folded into the invalidation reason. It was visible on the web and invisible on the desktop. - I/O gains ExtendTimeMs plus the three derivations the MCP tool already computed - avg read ms, a per-combination hit ratio, and each row's share of the window's read time (which is how the grid's order was decided, made legible). ContextMeaning moved to DarlingPgIoReader, beside the query that produces the value it explains, so the MCP surface and the viewer print one copy; it rides as the Context cell's tooltip because it is a paragraph. - The csproj comment's triple apostrophe, from my own shell escaping. Also filled while the pin was pointing at them: the autovacuum grid's ANALYZE half (a table can be vacuumed on schedule and still give the planner stale row counts), live tuples and the dead-tuple SHARE, the manual-vs-automatic run split, and autovacuum_count - zero being the classic wraparound route, since relfrozenxid never advances on a table autovacuum has never processed. Statements gain the database OID (half the read's grain), one cache-hit ratio over both Aurora cache tiers, and temp blocks READ beside written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…onitor into feat/2530-viewer-pg-tabs
The merge itself was clean, but it carried a SEMANTIC conflict a text merge cannot see: PgInt64IdentityWireShapeTests arrived on dev naming DarlingPgStatementReader and DarlingPgBlockingReader, which this branch had already moved to PerformanceMonitor.Darling.Storage. One using. Nothing about #2548's fix changes here — queryid was already rendered as a string in the viewer's statement projection, for the same reason it is now a string on the wire: a PostgreSQL int8 queryid routinely exceeds 2^53, and the one field whose entire purpose is joining back to pg_stat_statements must not go through anything that rounds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three note assignments indexed All[0..2], which couples them to the registry's STRIP order - a list whose whole job is to be reorderable. The first reorder would have put the Vacuum note above the Activity grids, silently and correctly-compiling. NoteFor(id) instead, returning empty for an id the registry does not carry, because a missing note must never take a tab down with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
IsPostgres is only true for a token DescribeEngineKind recognises, so inside that branch EngineDescription always has words - the local function offering 'an engine the store has not recorded' was describing a state this code path excludes, which is the kind of defensive branch that later reads as a real case somebody should handle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two small ones the prose already promised and the markup did not deliver. The Query Shapes tab gave the per-database counters TWICE the height of the statement grid they exist to explain, which is backwards: they are the follow-up question (did this spill?), not a second list of equal standing. Swapped. And an unrecognised engine token reaches the badge raw so an operator can search their store for it; raw did not have to mean untrimmed, and a hand-edited row with surrounding whitespace would have rendered it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
**The CI red is mine, and the guard that caught it did its job.** DarlingPgReadSqlParsesLiveTests discovers the shipped PostgreSQL reads by reflection, filtered on a HARDCODED namespace string — and this branch moved the readers, so the filter matched nothing and its own anti-vacuity floor failed the build saying exactly that: "the reflection filter has stopped matching, so this test is no longer checking anything". That assertion is the reason a silent pass was not what happened instead. The filter now takes the namespace from a reader TYPE it has to resolve anyway, so it follows the readers wherever they live. Verified the fix rather than assuming it: on macOS that test SKIPS before reaching its own count (no DARLING_TEST_PG), so "it passed" proves nothing — a throwaway host invoked the shipped private ShippedReadSql() directly and it discovers 12 constants across the nine readers, which is the number its own doc comment names. Proved red by restoring the literal: 0. **Two review findings, both real.** Bytes() chose the display unit against the UNROUNDED quotient and then formatted with N1, so one byte short of a megabyte rendered "1024.0 KB" — a number expressed in its own next unit, which reads as a typo rather than a size, on every column built over bytes. Re-checked after rounding. WPF applies the LAST matching DataTrigger, and IsInvalidated/IsInactive are not exclusive: an invalidated replication slot is usually also inactive, because its subscriber has already gone. Declaring inactive last painted the informational amber over the red on exactly the rows that needed the red. Swapped — and asserted as a RULE over every row style on these tabs rather than fixed on the one grid review found, since the same ordering pattern is one edit away from recurring: each flag carries a severity and a milder one may not follow a severer one. The Overview grid's pair is mutually exclusive today and is ordered that way anyway, so one rule covers the tab set instead of a rule and an exception. Both proved red by reverting only the thing under test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Give a PostgreSQL target PostgreSQL tabs in the WPF viewer (#2530)
…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>
…2546) The miss vocabulary had three words and none of them fitted a setup step somebody can change: `empty` claims we looked and there was nothing, `unavailable` sends the reader to collection health where they find a collector that is running and doing its best, and `not_collected` is the PERMANENT answer and tells them to stop looking. The store already held every one of these facts. The runners classify a denied grant, a missing source object and a disabled feature out of the general ERROR bucket and write the remedy into collection_log.error_message - the SQLSTATE 42P01 case says CREATE EXTENSION in so many words - and the hourly query_store_health collector records actual_state per database. No read reported any of it. Two Query Store reads were guessing in prose ("Query Store may not be enabled on target databases"), which is equally true of a server where it IS enabled and the window simply reached past what the raw tier retains. CollectorRuntimePrecondition is the shared vocabulary: the `precondition` status word and the two message shapes, with the remedy text FRAMED rather than authored - a second copy of prose whose whole value is being accurate about one server's answer is the copy that drifts. The noun phrase comes from CollectorEngineCapability.CapturePathByCollector rather than a second table. DarlingRuntimePrecondition and McpRuntimePrecondition are the two store halves and hold no vocabulary of their own, so the SKUs are byte-identical here by construction rather than by pinning. Evaluated at READ time, not gate time, and that is the whole point. An AppliesTo gate is decided once when the connection is made, so it would go on reporting a precondition after somebody satisfied it - the operator does what the message asked and nothing changes. This re-derives on every call. No sweep change and no IL-guard change: the capability derivation is untouched and this asks a different question of the store, not a new question of the gates. Wired on five reads per SKU - get_deadlocks, get_blocked_process_xml, get_running_jobs, get_long_query_completions, get_query_store_top - plus Darling's get_pg_top_queries, always AFTER the engine-capability answer so a permanent gap still wins. Nothing in the type system enforces that order and a `??` chain is one cut-and-paste from reversing it, so a source scan pins it across both trees, along with SKU parity and a non-vacuity floor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An in-place upgrade is an overlay: Expand-Archive -Force writes what the new build ships and deletes nothing else, so a file the old version had and the new one dropped stayed in the install tree forever - and DarlingInstallDirectoryReport walks top-level DIRECTORIES, so a stale DLL in the root, in viewer\ or in runtimes\ was structurally invisible to it. The measurement came first, because the issue's own decider was whether this has ever actually happened. Diffing the file lists of consecutive release zips: the Lite package dropped 44 shipped files across twelve consecutive releases - 43 in ONE step, where a target-framework move stranded every runtimes\*\lib\net8.0\ assembly including two copies of Microsoft.Data.SqlClient.dll - while the Darling package has dropped none in the four steps it has existed for. Rare, bursty, tied to a packaging change, and when it happens it lands forty files at a time in a .NET probing directory, where a stale assembly is a candidate for LOADING. The authority is a manifest DERIVED from the payload, never a list anybody maintains. After each successful copy the script records the files that copy laid down; the next upgrade diffs its own payload against it. So every path it can name provably came out of one of our own zips, which disposes of the whole false-positive class the obvious approach has: darling.json, the DPAPI blobs, the .bak-* config copies, the _rollback_manual_* backups and pg-runtime were never in a payload, so they are never in the manifest. That is a property of where the list comes from rather than a list of exceptions someone keeps current - which is #2525 again with a new subject. It fails safe and says so. No manifest yet, one that will not parse, one that disagrees with its own file-count, or a source it cannot read all mean remove NOTHING, on its own line, with the reason. -RemoveStaleFiles turns naming into deleting and is OFF for the first release: the check reports either way, and a delete inside a monitoring host's install directory should spend a few deploys showing operators its answer first. Files reported and not removed stay nominated in the next manifest, so the report is not a one-shot that forgets. Directories emptied by their own removal are pruned with NON-RECURSIVE deletes, deepest first. Not tidiness: the layout report identifies a satellite-resource directory structurally, an EMPTY one fails that test, and leaving the shell of de\ behind would turn one stale file into a permanent startup warning. Guards at three depths, each proved red by reverting only itself: the pg-runtime PREFIX rule (a prefix, not the two names we ship today, since an enumeration one entry out of date here costs the store), an empty payload selecting nothing rather than everything, an empty shipped-set refusing the delete outright, case-INSENSITIVE matching so a respelled App.js/app.js is never deleted right after the copy wrote it, and containment resolved against the filesystem rather than the string. Pinned by handing the shipped delete a deliberately POISONED list holding the store, the config, its backups, a credential blob, a rollback backup and a relative escape, then asserting against the DISK rather than the function's own account of what it did. No C# twin of the rule: nothing in the service participates in this convention, so a second implementation would be a copy that drifts while its test keeps passing. upgrade-darling.ps1 also gains a parse-and-AST pin, because $x | ForEach-Object { } -join ', ' binds -join as a PARAMETER, parses clean, and throws at runtime - which in this script means after the service has been stopped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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>
Doc comment only, no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
claude[bot] caught that `actual_state = 'ERROR'` fell through to the OFF wording. IsCollecting already excluded it correctly, but the bucketing right after only split READ_ONLY out, so an ERROR database was told "Query Store is not enabled ... turn it on with ALTER DATABASE SET QUERY_STORE = ON" - which is exactly the mistake the READ_ONLY branch exists to avoid, one state over. ERROR means Query Store WAS configured and stopped after an internal failure, so desired_state is typically still READ_WRITE, the suggested ALTER is already in effect, and re-running it silently no-ops while the actual repair goes unmentioned. Worse, with a mixed snapshot the sentence named the wrong database: the ERROR row was swept into the `off` bucket, so a scope holding BrokenDb (ERROR) and OffDb (OFF) produced "Query Store is not enabled on the database this read covers (OffDb)" and dropped the broken one entirely. There are three not-collecting states and each has its own remedy, so each now gets its own sentence, ordered by how badly the OFF wording would mislead. ERROR points at sys.sp_query_store_consistency_check first, then SET QUERY_STORE CLEAR or an OFF/ON cycle, and says both discard the data already held. Two tests, both proven red against the previous code: the ERROR sentence itself (including that it must NOT say "Turn it on with ALTER"), and that ERROR outranks READ_ONLY when a scope holds both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parse pin added in the previous commit went red on its first CI round, against lines nobody had touched, and it was right. upgrade-darling.ps1 did not parse under Windows PowerShell 5.1 at all - the default powershell.exe on Windows Server, and the one SSM runs. The file is BOM-less UTF-8; 5.1 decodes such a file with the machine's ANSI code page; the third byte of a UTF-8 em dash (E2 80 94) becomes U+201D in CP1252, which PowerShell honours as a CLOSING DOUBLE QUOTE. So every em dash #2528 put inside a double-quoted message terminated its string early, the rest of the line became code, and the script failed to parse before doing anything at all. Reproduced byte for byte on this machine, down to the same "The '<' operator is reserved for future use" CI reported. The five em dashes already in install-darling.ps1 and fetch-pg-runtime.ps1 were harmless only because they happen to sit in COMMENTS, where a mis-decoded character is never parsed. That is not a property anybody can maintain by eye, and it is one edit from being false. Fixed by making all three shipped scripts ASCII-only rather than by adding a byte-order mark. A BOM works and then decays: it is one careless save from being gone, and its absence is invisible. "No byte above 127" holds under every code page, does not care about the BOM, and is a rule anyone can check - including the extracted-function probes in Darling.Tests, which write lifted PowerShell to a temp file and run it under powershell.exe, where a non-ASCII byte would fail in a way that looks nothing like its cause. Both pins proved red by planting a single em dash back into one double-quoted string: the ASCII check names the line, and the file then fails to parse through the same CP1252 path 5.1 uses. The parse pin now covers all four Windows-side scripts rather than only the one this issue touches. Never shipped in a release - #2525 is in the same unreleased section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
…2529) Review finding: the rootedness test ran after ConvertTo-DarlingManifestPath had already stripped leading separators, so it could never fire. The bot was right that it was not exploitable - the same stripping degrades an absolute path into an ordinary subdirectory name before Combine sees it, so the classic "rooted second argument wins" Path.Combine pitfall does not apply, and Remove-DarlingStaleFiles' own containment check backstops it either way. It is still worth fixing rather than deleting, because a check that cannot fire is a comment wearing a guard's clothes, and the behaviour it was meant to produce is the better one. Asked of the RAW input, '\\fileserver\share\x.dll' is now REFUSED and reported, instead of being quietly reinterpreted as a relative 'server\share\x.dll' that could name a real file inside the install root. Three cases added to the shared table - UNC, root-relative backslash, root-relative forward slash - and the reordering proved red by moving the test back after normalization and watching the UNC path sail through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…econdition-miss A runtime-precondition miss vocabulary, evaluated at read time (#2546)
…gs-drift # Conflicts: # CHANGELOG.md
Two collectors, two reads, two schema rungs, and a Storage tab on both front ends. They ship together because both add rungs and would collide on StorageVersion.SchemaVersion otherwise. #2541 - pg_index_usage_stats (v84, daily, per database, writers only). The scan counts are the easy half; the half that makes it shippable is that "unused" is not "droppable", so every row carries whether the index backs a primary key, a constraint or the replica identity, whether it is partial, an expression index or INVALID, and its full definition. The read also reports scans over the STORED window, not just the server's lifetime counter - an index with nine million lifetime scans and none in ninety days is dead weight today, and no live query can see that. #2542 - pg_table_bloat_stats (v85, hourly, per database, writers only). The statistics-based estimate ships over pgstattuple on a measured margin (44 ms vs 860 ms over 2,001 tables), and the estimate is SUPPRESSED rather than captioned when its inputs cannot be trusted: stale statistics were measured 81 percentage points wrong on two byte-identical tables, and a pg_monitor-only role sees zero pg_stats rows and makes the estimator return a confident 88.59% for a table whose true bloat is 0.50%. Version floors measured against live PostgreSQL 13, 14, 15, 16, 17 and 18 rather than read from documentation. Exactly one exists: last_idx_scan is PG16+, proven red by running the gated-on form against 13/14/15. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ndex-bloat # Conflicts: # CHANGELOG.md
…moval # Conflicts: # CHANGELOG.md
) A store's tables come from one of two texts depending on when it was created: a fresh store builds them from V1's generated schema, walked from the collector catalog, while an existing store has whatever its rungs built. PgSchemaGeneratorTests enforces that the two say the same thing column for column, and adding the columns to PayloadColumns moved the generated side only. Both places are now updated and neither is redundant. The CREATE is IF NOT EXISTS and never re-runs on a store that already has the table, so the existing population needs V101's ALTER; the ALTER is ADD COLUMN IF NOT EXISTS and is a no-op on the fresh store that just created them. Dropping either would leave one population permanently without the columns -- the invisible, permanent divergence that test exists to catch. Verified locally rather than by another CI round: the generator's CREATE TABLE and the rung's are byte-identical after the normalisation the test applies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collect and serve PostgreSQL 18's measured I/O byte totals (#2655)
Version drift is silent from one server. 17 gutted pg_stat_bgwriter and 18 removed pg_stat_io's op_bytes, and in both cases a collector that guards the difference correctly and a read that quietly returns NULL look identical unless two majors are present at once. #2653 and #2655 were both found and verified that way, and the rig could not do it -- PG_MAJOR replaces the target rather than adding one. `docker compose --profile multiversion up -d` adds a plain PostgreSQL 18 on 55418, alongside the existing 17. Opt-in so the default stays a two-container rig. Deliberately the plain image, not the extension build: the PGDG extension packages do not track a new major immediately, so pinning to them would make the whole rig fail to build on the day a major ships -- exactly when this target is most useful. It exercises the core catalog views, which is where version drift lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add an opt-in PostgreSQL 18 target to the verification rig
…register ten MCP tools that shipped unreachable (#2659) pg_settings was never collected. SQL Server answers this three ways and PostgreSQL had no answer at all, so neither "what is work_mem set to here" nor "what changed last Tuesday" could be answered -- and the second is the kind that cannot be recovered afterwards at any price, because a configuration history nobody recorded is not sitting on the server waiting to be read. V102 stores the snapshot hourly, 365 days, and stores every column without filtering. source is what separates the server's configuration from the collector's own session -- pg_settings is a per-backend view -- and dropping a session-scoped row at collection time would make that evidence unrecoverable, so both reads filter instead, where it can be explained. Reporting one as a CHANGE would be worse than showing it: the collector reconnects, application_name moves, and the read would announce a change nobody made. get_pg_server_config reports what somebody actually chose, non-default first. Defaultness comes from PostgreSQL's own `source`, NOT from comparing setting to boot_val -- the string comparison invents non-defaults on a server nobody configured, measured on a live 17.11: data_directory_mode reads 0700 against a boot_val of 448, the same value in octal and decimal; archive_command reads (disabled) against an empty default; commit_timestamp_buffers reads 32 against 0 because 0 means auto-tune. That defect was in the first version of this read and only running it found it. get_pg_server_config_changes reports value changes between snapshots. A setting APPEARING is deliberately not a change: LAG returns NULL for the first snapshot of every setting, so without the guard the first collection would manufacture several hundred changes, and would again for every extension whose GUCs appear when its library loads. Both name pending_restart loudly -- the file edited and reloaded while the running server is still on the old value, disagreeing with no symptom until a restart months later changes behaviour during someone else's incident. #2659, found while wiring the above: six [McpServerToolType] classes -- ten shipped PostgreSQL reads -- were never registered with the MCP host. They were implemented, documented, dispatched by the web dashboard and counted in the census, and tools/list answered 116 where the census claimed 126. Nothing failed: the inventory pin checks NAMES, which exist either way, and the tab pin exists to stop a read shipping reachable only through MCP, which is the exact inverse of this. Registering them here rather than separately because two of the ten are this change's own tools, and shipping a feature into a class agents cannot reach is shipping nothing. A reflection-derived pin now asserts every tool class is registered, so it fails when a class is added rather than when an agent next reaches for the tool. Verified end to end against the rig through the real service: store at v102, tools/list 116 -> 128 with 25 PostgreSQL reads matching the census exactly, and after ALTER SYSTEM on the target the changes read reported exactly one change (work_mem 4096 -> 8192) out of 415 settings while shared_buffers correctly showed pending_restart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eight failures, every one a pin doing its job, and together they are the checklist for adding a PostgreSQL collector. The viewer half was the real work. EveryPostgresCollector_IsShownOnExactlyOnePostgresTab and EveryCollectorTable_HasAViewerReader_OrIsAllowListed both refused a collector the WPF viewer never shows -- the allow-list is a shrink-only ratchet, so the answer is a panel, not an entry. The settings grid sits under the extension axis on the Overview tab because it is the same kind of fact one layer in: extensions say what this server CAN do, settings say what it was told to do. It is the one panel on that tab NOT scoped to the toolbar window, because a configuration is the state now and an hours filter returns nothing for a server whose hourly collector last ran just outside it -- which reads as "no configuration" rather than "widen the window". The rest were counts a new collector legitimately moves: the pg rung list, the catalog count, the PostgreSQL tab count (7 -> 8, prose included), and the CapturePathByCollector noun phrase without which the capability message falls back to generic phrasing. The web tab used format: "datetime", which the renderer does not know -- an unknown format falls through to raw text, so the column still renders and the defect is invisible on inspection. It is "time". CiClusterWorkerSizingTests corrected a factual error in V102's own doc comment rather than just a number. I had written that pg_server_config is not a hypertable, arguing from its shape -- a snapshot of something a person changes, not a series of measurements. TimescaleSupport.HypertableTables is CollectorCatalog.All, so membership follows from being a collector and is not a per-table choice, and CI sizes its cluster's workers off that count. The doc now says so, and both workflows move to 70/81. Every pin re-validated locally against the product assemblies before pushing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both statements carried a SESSION_SCOPED token substituted at call time, which meant the constants were not SQL. DarlingPgReadSqlParsesLiveTests runs parse analysis on every shipped PostgreSQL read against a real server and both failed with 42703 column "session_scoped" does not exist. The deduplication was not worth it. A read whose text only becomes valid after a string replacement cannot be checked by anything -- not parse analysis, not a reader, not a person. The excluded sources are now spelled out inline in both statements, and PgServerConfigTests asserts each contains NOT IN ( plus the named constant, so the two cannot drift now that they are written twice. Verified by running PREPARE for both statements against a real PostgreSQL with their actual parameter signatures -- the same thing CI does -- rather than by rebuilding and hoping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
We had pg_stat_database.deadlocks -- a number that goes up -- and nothing else. PostgreSQL writes a complete report to its log: the wait graph, every participant, and each one's full statement text. In one respect that beats the SQL Server deadlock graph, which names the victim's statement and often leaves the other side as a handle. It needs NOTHING configured on the target, which is what separates it from plan capture. auto_explain needs a preload and a restart, which on a managed fleet is a parameter-group change nobody can make; a deadlock report is unconditional, and log_lock_waits governs ordinary lock waits rather than this. The only precondition is reading the log, which pg_plan_capture already established and pg_plan_capture_readiness already reports on -- so this works on the Aurora fleet today. Two things in the extraction were measured against a real 17.11 rather than assumed, and both are the kind that fail silently: %Q writes the query id with NO separator before the severity -- the captured line reads `[1549] 322048460535975151ERROR: deadlock detected`. A pattern requiring whitespace there matches nothing, in the way that looks like "this server has no deadlocks". The DETAIL block runs to the next line carrying a log prefix, and a participant's statement is arbitrary user SQL that can contain newlines, each arriving tab-indented. A line-count or blank-line rule truncates multi-line SQL to its first line, and the row still looks fine. The collector re-reads an overlapping tail on purpose so a report cut in half at one edge is whole in the next, and every row carries a hash of its graph text so the store dedupes. The hash is over the graph rather than (timestamp, victim_pid): the graph is what distinguishes two reports, and hashing the thing itself needs no argument about how unlikely a collision is. Participants are counted from the wait EDGES, not the statement headers, because the server omits a header when it could not recover the text and a participant with no statement is still in the cycle. Verified end to end against a real deadlock on the rig: the collector stored it with the same hash the parser produced offline, the reads returned the victim, the graph and both participants' SQL, and after a SECOND induced deadlock the store held three rows that the read correctly collapsed to two deadlocks -- one with times_seen=2 from the overlapping window, and the repeat as its own row because the process IDs differed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eachable (#2661) Three CI failures, all mine. PgSchemaGeneratorTests requires a collector's rung to be exactly what the generator emits, and the generator emits one index per collector -- so the extra dedupe index inside V103 was not a drafting nit the pin should tolerate. A fresh store builds from the generated schema and would silently not have had it. V103 is now byte-identical to the generated table and index, and V104 adds the index on its own, which gives BOTH populations what was actually wanted. get_pg_deadlock_detail was reachable only through MCP, which the POSTGRES_TABS pin exists to refuse -- and the fix is better than an exemption. The read took a mandatory hash, so it could only ever be a drill-down; it now takes an OPTIONAL one and answers with the most recent graphs without it, which is the shape the SQL Server "Deadlock Graphs" panel already has. A reader no longer has to call the summary first just to see a graph, and the tab gets a panel with no drill-down plumbing. DISTINCT ON (deadlock_hash) for the same reason every read here groups: the overlapping window means the newest rows are often one report several times. The viewer schema sentinel was simply missing -- my own checklist item, and the one that would have made the connect-time gate refuse a perfectly current store. Both rungs get an arm: V103 a table sentinel, V104 an index one, because a store stopped between them has the table and not the index and that is a real interrupted-upgrade state. Verified against the live rig: both statements PREPARE with their real parameter signatures, and the optional-hash read returns two distinct graphs with no hash and one with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collect PostgreSQL deadlock reports from the server log (#2661)
Fourteen trend reads shipped and none worked on a PostgreSQL target, so every PostgreSQL answer described one window and none answered "is this getting worse". This was a READ gap, not a collection gap: the rig held over a thousand snapshots each of pg_io_stats and pg_database_stats after two days, and nothing differenced any of it. get_pg_wait_trend follows one wait event over time; get_pg_query_duration_trend follows one statement's cost per execution, which is the regression read -- a query that doubled halfway through the window still ranks where its average puts it in a single-window grid. Three decisions worth stating, two of them corrected by running it: A counter going backwards is a RESET, not negative work. pg_wait_sampling_reset_profile() and a server restart both zero the profile, and the second happens without anyone deciding to. GREATEST(delta, 0) would report a QUIET interval across a restart -- the one reading that is definitely wrong, because the server was not idle. The interval takes the new value whole and says so per point. This follows DarlingPgWaitSamplingReader rather than inventing a second rule. The query trend reads the delta_* columns the collector already wrote rather than differencing the cumulative ones again. They are not equivalent: the collector's delta spans the interval it OBSERVED, a LAG here spans the gap in the STORED data, and whenever a snapshot is missing only the collector's is about the server. The automatic wait choice skips the CPU class. My first version picked whatever had the most samples, which on the rig was `Running` -- pg_wait_sampling's name for the backend NOT waiting, growing 1,534 samples against 194 for the next event. A wait trend defaulting to the not-waiting state answers the opposite of the question asked. It stays askable by name, because CPU time is a real signal; it is just never the automatic answer. Only running it showed this. Both subjects are optional and the read says which it chose, so a caller never mistakes an automatic pick for an answer about the event or query they had in mind -- and it makes both reachable as web panels without drill-down plumbing. Verified end to end through the real service: the wait trend returned 12 hourly points with per-second normalisation, the query trend 353 points correctly reporting a null mean for the intervals with no calls, and after the CPU fix the default moved from `Running` to `transactionid` while `Running` still answers when named. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ngs (#2663) The panel landed in SERVER_TABS. Both registries have an `io` tab and SERVER_TABS comes first in the file, so anchoring on the id matched the SQL Server one -- the same trap that put four panels on the wrong registry earlier in this work. The reachability pin caught it, which is what it is for: a get_pg_* read on a SQL Server tab is not merely misplaced, it is unreachable from the registry a PostgreSQL server actually renders. Anchored inside the POSTGRES_TABS region this time rather than on a tab id that exists in both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add the first PostgreSQL trend reads (#2663)
Set DARLING_OUTPUT_FORMAT=gcf to have the MCP server return Graph Compact Format instead of JSON. A single call-tool filter (registered once via WithRequestFilters) re-encodes each tool's JSON result as a GCF generic wire, factoring the repeated field names of the record arrays these tools return (blocking pairs, wait stats, alerts, config, ...) into one header and dropping the indentation — roughly halving the token cost of a result. Covers every tool with no per-tool changes. Conservative per result: used only when the GCF wire is both smaller than the JSON (never-grow) and a stable round-trip of it, comparing number-exactly (int64 preserved, not float-rounded); on any parse/encode/mismatch the JSON is kept, so a result is never grown, dropped, or garbled. Default JSON output is unchanged. BlackwellSystems.Gcf is zero-dependency, pinned exact.
- The fail-safe now compares the decoded wire against the parsed input model (ValuesEqual), not the wire against itself, so a value that survived only as a rounded double is caught and the result falls back to JSON. Key order is compared order-insensitively (header-order normalization is semantically equal for JSON objects). - FromJson declines any non-integer number a double cannot hold exactly (checked via TryGetDecimal); 33.5 / 0.25 still encode, a high-precision decimal or a uint64 past Int64 stays JSON. - Transform returns a new CallToolResult (carrying StructuredContent / IsError / Meta) instead of mutating the tool's result; drop the unused System.Linq. - README: savings are payload-shaped, document the - (null) / ~ (absent) sentinels, and scope the losslessness claim to what round-trips. - Revert the unrelated deprecated/Dashboard version bump.
NumbersEqual compared a long against an integer-valued double by widening the long to double, which rounds identically on both sides above 2^53 and would accept a wire that no longer carries the input value. In the mixed branch, compare without widening: the double must be integral, in long range, and convert back to the same long. Add a test that decodes the wire and asserts it reproduces the parsed input's values (the property the runtime enforces), rather than only asserting the wire re-encodes to itself.
The representability check used (decimal)d != exact, but a decimal cast keeps only 15 significant digits, so a 16-digit shortest-round-trip double such as 0.5029000043869019 (an exactly representable float8 value) was wrongly declined, sending real payloads back to JSON for no reason. It also failed open on a token that overflows double (1e400 -> Infinity), which the round-trip fail-safe cannot catch (Infinity == Infinity). NumberSurvivesAsDouble now declines non-finite doubles and accepts a value when its shortest round-trip form reproduces the token or the in-range decimal is unchanged. Tests pin 0.5029000043869019 as accepted and 1e400 as declined.
…utput Since the MCP tools serialize compact (WriteIndented=false), the sample payloads now build compact too, so the size assertions measure GCF against what the server actually emits rather than the pretty-printed JSON it stopped producing. GCF still wins on the uniform record shapes (Blocking 6757->2633; an integer proxy of the decline payloads is 710->308, so a declined result is the precision guard, not the never-grow guard). Class comment updated to state the win against compact JSON.
… role The roles already existed with distinct grants; only the generated pg_hba line was single-valued. NormalizeNetworkRoles returns an ordered, de-duplicated list, the plan carries it, and the managed block gets one hostssl line per role -- so tightening allowFrom still narrows all of them, which a hand-added line outside the markers would not do. Verification compares the live rule COUNT to the number of roles rather than checking one is present: a rule that applied for one role and not the other would otherwise leave those clients locked out with the exposure looking healthy. Tests and the --configure-network prompts still to come. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Mechanical: decision.Role -> decision.Roles?[0]. These assert on a single-role exposure, which is still the default and still the shape they were written for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#2665) Finishes the role list and fixes a defect in the verification the WIP added. VerifyPgHbaAsync counted ROWS matching any admitted role and compared that to the number of roles. Rows do not answer the question: two rules naming the SAME role satisfy the count, and that is the expected shape on an upgraded box, because #2665's documented workaround is a hand-added second hostssl line outside the markers that ReconcilePgHba deliberately preserves. Such a file with a stale viewer line and no live admin rule counted 2 of 2 and logged "reconciled" while every admin client was locked out - precisely the failure the check exists to catch. It now counts DISTINCT role names via unnest, which reaches Roles.Count only when every admitted role really has a live rule. NormalizeNetworkRoles: a value of only separators (",", "+") no longer reads as the viewer default. It is not the blank case - the operator wrote something and none of it names a role - so it fails closed like a typo instead of exposing a store off a malformed field. NormalizeNetworkRole also guards the empty list before indexing it; it cannot happen today, but it is the one caller that would index, and every other caller already checks. --configure-network offers both at the prompt (default still viewer) and keys its admin warning on the NORMALIZED value, so the warning that matters most - the one for an operator who typed "admin,viewer" while thinking about the viewer half - does not go silent. --print-viewer-connection prints one paste-ready string per admitted role, each labelled with the seat it authenticates as, with the shared certificate emitted once and every STDERR warning still ahead of the first STDOUT byte. --export-viewer-config writes ONE folder holding ONE live credential, so where both roles are admitted it picks the least-privileged seat and says so rather than choosing silently. DarlingWorker's D7 pivot warning now says the role "admits 'admin'" rather than "is 'admin'", which would read as wrong to the operator who configured a list and invite them to dismiss it. Also strips the UTF-8 BOMs the WIP commit added to five files; 134 of the 147 .cs files in the service carry none. Verified: whole-solution build clean on macOS. The Windows-only suites cannot run here, so the pure logic was run against the real build in a throwaway net10.0 harness - 115 checks mirroring every new assertion, all green, and red in the right places when the stable sort and the whole-value rejection were each removed.
feat: add opt-in GCF output format for MCP tool results
…posure Store exposure admits both roles at once (#2665)
The remaining two trends from #2663. `pg_io_stats` and `pg_database_stats` are the deepest series the store holds - over a thousand snapshots each on a two-day-old rig - and nothing differenced them. `get_pg_io_trend` follows one (backend_type, context) pair. A PAIR, because a hit ratio summed across contexts is meaningless: bulkread is a sequential scan deliberately using a ring buffer so it cannot evict the pool, and folding its misses in with the normal context's understates both when the two have opposite remedies. Measured on the rig, that pair's hit ratio is 2.5% - correct, and unreadable if it were averaged with anything else. Object types ARE summed, which cannot distort the ratio because WAL rows report no hits. `get_pg_database_trend` follows one database's spills, hit ratio, deadlocks and rollback share. The hit ratio is the reason it exists: pg_stat_database's counters are cumulative since the last reset, so the ratio computed from them raw is a lifetime average that barely moves. On the rig a database averaging 98.78% across the window had an interval at 78.49%, and on the PG18 target 85.28% hid a 61.52%. Only the differenced series can say so. Three decisions that running it changed or forced: - The subject choice is ranked on OPERATIONS, not read time. The single-window read orders by read_time_ms, which is the better ranking when populated - and it is usually not: track_io_timing is off by DEFAULT and was off on both rig targets, so every read_time_ms in the store is 0.0 and ranking on it resolves to picking a name alphabetically. - Both reads now ask the target's own track_io_timing before reporting a latency. read_time_ms / reads over an untimed server is 0.000 ms, which reads as an impossibly fast disk rather than an unmeasured one - a confident wrong answer about the one number an operator would act on. - Half a subject is a real request. Naming only backend_type constrains the choice and the context is picked within it, rather than the named half being silently dropped. Neither table carries delta_* columns - checked against the stored schema, not assumed - so both difference with LAG. A counter going backwards is a RESET and the interval takes the new value whole and is flagged; GREATEST(delta, 0) would report a quiet interval across a restart, which is the one reading that is definitely wrong. Both reset signals from the single-window database read are carried, because neither sees every reset. Verified on the rig, against both PostgreSQL majors it holds: all 43 shipped PostgreSQL reads pass PREPARE parse analysis; the reset and late-arriving-series arithmetic proven against controlled rows with the shipped text unmodified; both reads exercised end to end through the service; and both panels proven to be fetched from POSTGRES_TABS and from no SQL Server tab. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…#2663) Three things, all found by reviewing and re-running what the first commit shipped. **A fully cached server had no I/O trend at all.** The automatic subject required reads + writes + extends > 0, so on a workload PostgreSQL is serving entirely from shared_buffers - the healthy state - every combination has hits and no physical I/O, the choice matched nothing, and the read answered "nothing to follow" about a server that is perfectly observable. Measured: a six-hour window on the rig's quieter target has 0 operations and 248,293 buffer hits. Hits now QUALIFY a pair, and break the tie ahead of the name, while operations still rank it - so the 24-hour windows still pick the pairs that actually move I/O (bulkread with 8,824 reads on 17, checkpointer with 36,076 writes on 18) and the six-hour one now draws the 100%-hit-ratio chart it was refusing to. Without the hits tiebreak that window would fall through to alphabetical order, which is the same defect ranking on an unmeasured read_time_ms would have caused. The single-window database read's choice already had this right. The I/O one did not, and the asymmetry is what gave it away. **Decision pins for both new reads**, in the file #2664 established for them: the LAG-not- delta call and why it is the opposite of the query trend's, the take-it-whole reset rule on both series, per-second normalisation, the operations-not-read-time ranking, the pair-shaped subject, the shared-relations NULL, the 18-vs-pre-18 byte split, and that a quiet interval is still a point. Each was run against a deliberately broken copy of its own SQL first: a pin that passes before and after the fix converts an open question into false confidence. **And the pins had to stop reading the prose.** Two of them were red against CORRECT SQL, because the comment explaining a decision quotes the string the pin forbids - the one saying why there is no HAVING, and the one saying why the ranking is not on read_time_ms. Both comments are load-bearing and neither should be reworded to appease a test, so every "must not contain" now runs against the query with its block comments stripped. Fixed as a category after hitting it twice. Also: PgIoTrendPoint and PgDatabaseTrendPoint are sealed records rather than record structs, on the size rule DarlingPgIoReader.PgIoRow already follows - a twenty-four-field struct is copied whole by every LINQ pass over a thousand-point series. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-trends Add the PostgreSQL I/O and database-stats trends (#2663)
<Version> 3.5.0 -> 3.6.0 in the three shipping products and the deprecated Dashboard the version-bump gate checks. Deliberately JUST the version bump. The CHANGELOG heading stays [Unreleased] and no migration-ladder-v3.6.0 fixture is added at the cut: the upgrade-path guard (MigrationUpgradeLadderLiveTests) requires the most-recent-RELEASED fixture to be the store users upgrade FROM, always behind the current ladder. Promoting the heading to [3.6.0] now would make the release its own previous version and the climb would apply zero rungs. The heading promotion and the v3.6.0 fixture land next cycle once the ladder moves past v104 -- exactly how #2572 added the v3.5.0 fixture four days after 3.5.0 shipped. The GitHub release notes are authored from the [Unreleased] section. Schema upgrade v79 -> v104 verified end to end against a real v3.5.0 store. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Release 3.6.0
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
3.6.0 → main.
check-version-bumpverifies 3.6.0 > 3.5.0 (deprecated/Dashboard/Dashboard.csproj).Merge this NOT squashed — the release build and history depend on the individual commits.
Scope since 3.5.0
194 merges. Store schema v79 → v104 (25 rungs; full upgrade verified end to end against a real v3.5.0 store — climbed to v104, matched the constant, re-ran idempotent).
Headlines: PostgreSQL parity (configuration + change history #2658, deadlock capture #2661, all four trend reads #2663, PG18 support #2655, major-version persistence #2653, multi-role store exposure #2665), ten previously-unreachable PostgreSQL MCP tools now registered with a reflection pin #2659, and the opt-in GCF output format (#2286, external — 29.7% fewer tokens on real payloads).
Plan capture (#2538) ships as built and tested to the fleet's limit: proven end to end self-hosted, RDS transport parse pinned against real bytes, AWS calls faked-client tested. The untested hop needs an Aurora
auto_explainpreload + reboot unavailable on this fleet — a fleet constraint, not a product one.After merge
Publish the GitHub release with tag
v3.6.0to trigger the build; it stops at the SignPath approval gate. The CHANGELOG heading promotion to[3.6.0]and themigration-ladder-v3.6.0fixture land next cycle once the ladder moves past v104 (the upgrade-path guard requires the most-recent-release fixture to sit behind current — same timing as #2572 for 3.5.0).