Fixes #2800: document Lite's server-scoped watermark read as deliberately unbounded

CHANGELOG.md

@@ -100,6 +100,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **The web dashboard's server page gets the viewer's tabs: twelve sections, 61 of the 82 served reads, and a time range** ([#2475]) - the service dispatched **82** reads at `GET /api/read/{name}` and the built-in pages reached **23** of them; `#/server/{name}` was one scroll of seven panels against the desktop viewer's **65** `TabItem`s. The gap was never backend work - `panels.js` has been a generic renderer over those reads since #1562 - so this is descriptors over the unchanged `renderPanel` seam: twelve sub-tabs (Overview, Wait Stats, CPU, Memory, Blocking, File I/O, Queries, Configuration, Config Changes, Activity, System Events, Collection Health) carrying ~70 panels, reaching **61** reads. The header carries the WHY beneath the band badge - `Warning` has three unrelated causes (a real metric breach, a server awaiting its first collection, a collector error), so a badge reading "Warning" with no way to ask why is [#2422] rebuilt on a new surface; the fleet's own reason string and the fleet card's severity chips are rendered there, the chips through `fleet.js`'s own `metricBands` so there is one implementation rather than two, and neither is re-derived in the browser (R1). Every web grid that renders query text now puts it immediately right of its time/identity anchor ([#1949]) and the pin that enforced that on one array enforces it on all seven. The tab id rides in the hash (`#/server/{name}/{tab}`) so a section is deep-linkable and survives the 60s refresh, and an unknown or absent id resolves to Overview, which is what keeps every existing `#/server/{name}` link working. A page-level range picker (1h/4h/12h/24h/7d/30d) is the twin of the viewer's toolbar presets - **not** persisted, because a page that reopens on a 30-day window is slow for a reason the reader cannot see; panels whose read takes no window at all say "latest snapshot" rather than inheriting a label that would misdescribe them. Two reads were previously **unreachable from a browser** because they require a parameter no UI collected - `get_wait_trend` needs a `wait_type` and `get_perfmon_trend` a `counter_name` - and both now have a picker, the wait one seeded from the rows of the table directly above it so the picker and the table cannot disagree. **No fifth viz kind was added**: the property-grid shape that tempted one is served by `stat` (the reads returning a flat object) and `table` (the ones already returning rows), and a fifth kind that lived only in `panels.js` would be a page-only special case, while doing it properly means `KnownVizList`, `derive.js` and an editor config arm - composer surface this change does not need. What the browser genuinely cannot do is **stated in the tab where a reader goes looking for it** rather than left as a page that quietly lacks a feature: plan analysis, the query heatmap, cached-plan retrieval and actual-plan re-execution need a plan renderer and a command back to the monitored server, and the block-chain view and interactive deadlock graph need a graph viewer - the Blocking tab hands over the captured blocked-process-report and deadlock-graph XML verbatim instead of pretending. Every data panel supplies its own empty-state sentence and both helpers THROW without one. vizTable's generic "No rows in this window" reads as a fault on a collector that is off, opt-in, or daily; and the chart case was worse - `get_blocking_trend` and `get_deadlock_trend` answer an IDLE server with `trend: []` and no `{status,message}` envelope at all, so a perfectly healthy server was told its blocking chart did not have "enough data points to chart yet". `vizLine` now renders a descriptor's `emptyText` at exactly ZERO rows and still falls through at one (where the chart's own sentence is the true one) and when no `emptyText` was authored, so every stored view predating this is unchanged. A read feeding several panels on one tab is fetched ONCE (`fanout`) rather than per descriptor - `readTool` has no cache, so `get_collection_health`, which rolls up seven days of collector logs and computes sweep pressure, was running three times to open its own tab; six such duplicates existed across five tabs and a pin now refuses a seventh. Guarded by an invariant rather than by spot-checks: every read name the module mentions must exist in the shipped dispatch, every parameter key must be one its read actually binds (an unknown query key is silently ignored, so `limit` sent to a read binding `top` quietly returns the default), every viz must be in the shipped vocabulary, and no `get_pg_*` read may appear until the fleet payload can tell a PostgreSQL target from a SQL Server one - it carries `engine_edition`, not a `CollectorTargetEngine`, so a PostgreSQL panel today would render on all 42 SQL Servers, permanently empty.
 
 ### Changed
+- **Lite's server-scoped watermark read stays UNBOUNDED, and the asymmetry with its per-database twin is now documented as deliberate** ([#2800]) - filed as the Lite half of [#2795], on the reasonable read that `GetLastCollectedTimeForDatabaseAsync` taking a `collectedSince` bound while `GetLastCollectedTimeAsync` does not is the same oversight [#2344] left on the Postgres side. Measured against DuckDB rather than inherited from Darling's numbers, it is not. **The failure [#2795] fixed cannot occur here**: its mechanism was Npgsql's undocumented 30 s default `CommandTimeout` cancelling a 40,743-50,560 ms read, the cancellation being swallowed, and the resulting null being indistinguishable from a first run - silently downgrading the collector to its fallback window. `DuckDBCommand.CommandTimeout` defaults to **0**, meaning no limit, so there is no ceiling to exceed and nothing to cancel. **And the bound does not pay.** Measured on DuckDB 1.5.5 (the version Lite ships) against this table's shipped generated DDL at 50M rows / 4.7 GB - far past any realistic Lite store - with the connection already open so the figure is query cost rather than connect cost: with ONE monitored server, the common Lite deployment where `server_id` selects everything, unbounded is **0.34 ms** and bounded is **0.56 ms**, a 65% LOSS, because DuckDB answers the unfiltered `MAX` from column zonemap metadata and the `collection_time` predicate forces real evaluation instead. With five servers the bound is a genuine 9.8x (8.26 ms to 0.84 ms) but saves ~7 ms once per five-minute cycle. **The per-database twin's bound is earned and stays**: 8.40 ms to 0.79 ms at one server and 10.42 ms to 0.93 ms at five, roughly 10x in BOTH shapes, because `database_name` is not in `idx_query_store_time(server_id, collection_time)` so that `MAX` genuinely scans and genuinely prunes. The two reads have different cost structures in a columnar engine; in Postgres both scanned every chunk, which is why the shapes match there and diverge here. Verified across 1M-50M rows, narrow and realistic payload widths (identical, as expected when the query reads three narrow columns), and correlated vs scattered `last_execution_time` - the conclusion holds on all of them. No behaviour change: the method's doc comment now carries the measurement and names `CommandTimeout`'s default as the load-bearing assumption to re-check.
 - **The SQL-Agent collectors no longer gate on msdb access, so running the GRANT we advise actually does something** ([#2559]) - `HasMsdbAccess` is a **grant**, not an engine capability, but it was probed once at connect and cached for the connection's life. Three collectors gated dispatch on it, so the sequence a user actually follows was broken end to end: read our advice, run the `GRANT`, and nothing happens until the service restarts, with nothing to indicate why. `running_jobs`, `job_history` and `agent_status` now attempt regardless and fail into `PERMISSIONS`, which is a first-class outcome here rather than a defect - error **916 is already in `SqlServerPermissionErrors`**, so the run is classified as a permission denial and never as an ERROR, and `CollectorHealthClassifier` bands a collector that has only ever been denied as `NO_PERMISSIONS`, a check that runs **before** FAILING and STALE. So a server that will never have the grant does not read as broken, raises no alert, and the grant now takes effect on the next cycle instead of the next reconnect. The cost this trades for is three fast-failing statements per cycle on such a server - a compile-time permission check with no execution behind it. The fleet number that made the call: across both stores over three hours, **all 84 servers already dispatch all three collectors with zero non-SUCCESS runs**, so `HasMsdbAccess` never fires here and removing it changes nothing on this fleet while fixing the journey for anyone it does fire on. `HasMsdbAccess` is kept and still probed - it is honest information for a connection surface - it just no longer decides dispatch. One pin was **deleted rather than left passing**: the PostgreSQL-leak guard asserted no SQL Server gate reads `HasMsdbAccess`, which is now true for a reason that has nothing to do with the filter it was written to test, so it could no longer go red and was reading as coverage it did not provide.
 - **`get_pg_top_queries` and `get_pg_blocking` return their int8 IDENTITIES as strings, because a JSON number was rounding them** ([#2548]) - PostgreSQL's `queryid` is a signed `int8` derived from a hash, so its values are spread over the whole 64-bit range and most of them sit past 2^53. Serialized as a JSON **number**, every parser that decodes numbers as IEEE-754 doubles - `JSON.parse`, `json.loads`, most agent tooling - silently rounded one: `-4185925123159566327` came back as `-4185925123159566300`, verified against real `JSON.parse`. The value was never wrong on the wire; it was unrecoverable after parsing, which for an identity is the same thing. **This is a breaking change to the response shape and it is the right trade**: `queryid` is the ONLY identity a PostgreSQL statement has, and every use of it - `SELECT ... FROM pg_stat_statements WHERE queryid = ...`, matching a row on our screen against 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. **`get_pg_blocking`'s `root_backend_id` had the same defect in a worse form**, and it is fixed in the same change rather than noted for later. The collector builds that id by CONCATENATING the backend's start epoch with its zero-padded pid, so every value is a 17-digit integer around 1.79e16 - roughly 2x past 2^53, where adjacent doubles are 2 apart. Half of all backend ids are therefore 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 on four adjacent pids, two of the four collided; over 200, 100 lost precision and 99 landed on an id belonging to another backend. That is a worse failure than `queryid`'s - joining to the wrong row rather than to none - in the field the tool's own description tells a reader to prefer for comparing a root blocker across captures. The fix is scoped to those two: `database_id` and `user_id` are PostgreSQL `oid`s (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 whose real values are nowhere near 2^53 - so none of them has a defect to fix and none was touched. Web rendering needed no change: the Query ID column already rendered the raw value, so it simply started showing the true digits. Guarded by `PgInt64IdentityWireShapeTests`, which pins both fields as strings, pins `database_id` and `root_pid` as numbers so "stringify everything" cannot pass, and asserts its own fixture is genuinely out of double range - the assertion that stops the guard being vacuous.
 - **Fifteen reads stopped answering "nothing happened" and "nothing was collected" with the same sentence** ([#2485]) - a read that returns no rows was saying so identically whether the window was genuinely quiet or the collector had never run, and those want opposite responses: widen the window, versus go find out why collection is not running. They now use the miss vocabulary honestly - `empty` for a true negative, `unavailable` for data that was never collected - with the second saying outright that it is NOT an all-clear and naming what to check. The design rule that came out of it is worth stating: **for an EDGE table the denominator is whether we LOOKED, not whether we ever FOUND anything**. Blocking and deadlocks only write a row when something went wrong, so probing the data reports a server that has been collected perfectly for months and simply never blocked as uncollected - a false alarm sending someone to fix collection that works. Those reads count successful collector runs instead. A parity pin now asserts each shared sentence appears in both source trees, because every one of these messages exists twice and nothing previously stopped one copy drifting.
@@ -3152,6 +3153,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#2785]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2785
 [#2791]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2791
 [#2795]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2795
+[#2800]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2800
 [#2801]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2801
 [#2803]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2803
 [#2804]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2804

Lite/Services/RemoteCollectorService.cs

@@ -1284,6 +1284,38 @@ protected internal static int GetServerId(ServerConnection server)
     /// <summary>
     /// Gets the most recent value of a timestamp column from DuckDB for incremental collection.
     /// Returns null on first run or if the query fails (caller uses a fallback window).
+    ///
+    /// <para><b>This read is deliberately UNBOUNDED, unlike its per-database twin
+    /// <see cref="GetLastCollectedTimeForDatabaseAsync"/> and unlike Darling's server-scoped
+    /// equivalent.</b> That asymmetry looks like the oversight #2344 left on the Postgres side and
+    /// #2795 later fixed there, and it was filed as such (#2800). It is not: measured on DuckDB, the
+    /// bound does not pay here and in the common single-server shape it actively costs.</para>
+    ///
+    /// <para>Measured on DuckDB 1.5.5 (the version Lite ships) against this table's SHIPPED generated
+    /// DDL, 50M rows / 4.7 GB — far past any realistic Lite store — with the connection already open so
+    /// the figure is query cost, not connect cost:</para>
+    ///
+    /// <list type="bullet">
+    /// <item><b>One monitored server</b> (the common Lite deployment, where <c>server_id</c> selects
+    /// everything): unbounded <b>0.34 ms</b>, bounded <b>0.56 ms</b>. The bound is a 65% LOSS — DuckDB
+    /// answers the unfiltered <c>MAX</c> from column zonemap metadata, and adding a
+    /// <c>collection_time</c> predicate forces real evaluation instead.</item>
+    /// <item><b>Five monitored servers</b>: unbounded 8.26 ms, bounded 0.84 ms — a real 9.8x, but it
+    /// saves ~7 ms once per five-minute query_store cycle.</item>
+    /// </list>
+    ///
+    /// <para>The per-database twin's bound IS earned and stays: 8.40 ms to 0.79 ms with one server and
+    /// 10.42 ms to 0.93 ms with five — roughly 10x in BOTH shapes, because <c>database_name</c> is not
+    /// in <c>idx_query_store_time(server_id, collection_time)</c>, so that <c>MAX</c> genuinely scans
+    /// and genuinely prunes. The two reads have different cost structures in a columnar engine; in
+    /// Postgres both scanned every chunk, which is why the shapes match there and diverge here.</para>
+    ///
+    /// <para><b>And the failure #2795 actually fixed cannot occur here.</b> Its mechanism was Npgsql's
+    /// undocumented 30 s default <c>CommandTimeout</c> cancelling a 40-50 s read, the cancellation being
+    /// swallowed, and the resulting null reading as a first run — silently downgrading the collector to
+    /// its fallback window. <c>DuckDBCommand.CommandTimeout</c> defaults to <b>0</b>, meaning no limit,
+    /// so there is no ceiling to exceed and nothing to cancel. Confirm that default still holds before
+    /// concluding from this comment; it is what the whole argument rests on.</para>
     /// </summary>
     protected async Task<DateTime?> GetLastCollectedTimeAsync(
         int serverId, string tableName, string columnName, CancellationToken cancellationToken)