Propagate ComposeStatementTimeoutSeconds through ApplyToConfig, with a family-derived pin (#2917)

CHANGELOG.md

@@ -117,6 +117,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Lite's portable ZIP is self-contained, which HALVED it** ([#2501]) - `Publish Lite` is now `-r win-x64 --self-contained` in both `build.yml` and `nightly.yml`, so neither Lite artifact has a .NET prerequisite any more and the failure [#2489] documented stops existing: a tester who unzips onto a stock Windows Server no longer meets the .NET host's bare `You must install .NET to run this application` before a line of our code runs. **The size went the opposite way from what bundling a runtime suggests.** The old publish was RID-agnostic, so it copied every platform its packages ship - **537 MB of `runtimes\` on a 565 MB tree** (osx 130, linux-x64 116, linux-arm64 70, win-arm64 56, then win-x86, musl, loongarch64 and riscv64), of which only the **52 MB `win-x64`** folder could ever load on Windows. `DuckDB.NET.Bindings.Full` is most of it, SkiaSharp and SqlClient behind it. Dropping ~485 MB of unloadable native payload beats the cost of bundling .NET, WPF and ASP.NET Core by roughly two to one: measured on one commit and one SDK, **565 MB tree / 212.7 MB zipped becomes 277 MB / 114.2 MB**. It matters most for the **nightly** ZIP, which is the UAT download and is not offered as a `Setup.exe` at all. **A RID-specific publish needed two more files than the flag.** `Lite/packages.lock.json` had only a `net10.0-windows7.0` target, and a RID restore adds `net10.0-windows7.0/win-x64` to it - after which the `dotnet restore --locked-mode` that BOTH workflows run before the publish fails `NU1004: the project's runtime identifiers have changed`, because locked mode compares the PROJECT's RID set (empty) against the lock file's (win-x64). Reproduced locally; that is a red CI run on every PR, not the future `--no-restore` trap it was filed as. The fix is `<RuntimeIdentifiers>win-x64</RuntimeIdentifiers>` in `PerformanceMonitorLite.csproj`, so the project itself asks for that graph and one committed lock file satisfies the RID-less locked-mode restore and the RID publish alike; `RuntimeIdentifiers` (plural) sets no RID on the build, so a plain `dotnet build` stays RID-agnostic and `Lite.Tests` is untouched. **SignPath needed nothing** - the `Lite` artifact-configuration slug already receives both shapes today, and the signed re-zip reads `signed/Lite/*`, inheriting whatever shape `publish/Lite` has. Auto-update is unaffected; the ZIP is not a Velopack channel. `LiteRuntimePrerequisiteDocsTests` went red on the flag alone (3 of its 7 facts) and was rewritten to state every claim BOTH ways round: [#2499]'s version asserted only that the docs DID name the runtimes, so two of its facts stayed green while the prose went stale. It now also derives the lock file's RID coverage from the `-r` flags in the workflows, and every new assertion was proven red with its fix reverted.
 
 ### Fixed
+- **A control-plane reload no longer drops the compose `statement_timeout` knob on the floor** ([#2917]) - `StoreConfigProvider.ApplyToConfig` assigns every scalar in `StoreConfigView` that has a same-named `DarlingConfig` property, and `ComposeStatementTimeoutSeconds` was the **one exception** out of seven - seeded to `config_service`, selected by the view read, clamped to [5,600], mapped into the view, and then never assigned, so `config.ComposeStatementTimeoutSeconds` reported the `darling.json` value for the life of the process while the store said otherwise. **This did not change the effective ceiling**, and the distinction is the whole finding: nothing reads the held property, because `DarlingManagedRoles.ReadComposeStatementTimeoutAsync` reads `config_service` with its own `SELECT` and bakes the value into `ALTER ROLE viewer/mcp SET statement_timeout` at startup provisioning. So an operator's change was delivered on the next managed start all along - [#2357]'s stated design, and what the doc comment on `StoreConfigView.ComposeStatementTimeoutSeconds` has always said. What was broken was the held config's **truthfulness**, made worse by the `DarlingConfig` doc comment inviting a reader to trust it ("the store is authoritative afterwards, like every other value here"): a future consumer reading the held value would have silently gotten the file value. That comment now states the restart-scoped delivery, and the *delivery* question - a reload observes the new value without re-asserting it onto the roles, unlike every other `config_service` knob - is filed separately as [#2918] rather than changed here. **The pin is family-derived, not another per-knob assertion** ([#2917]): every existing test on this path names its own knob, which is exactly why the seventh was dropped and why the eighth would be, so `StoreConfigViewPropagationTests` reflects over the real types and round-trips **every** view scalar with a same-named, same-typed, settable `DarlingConfig` property through `ApplyToConfig` - two probe values apiece, each pre-setting the held config to the other so an absent assignment cannot pass by landing on a shared default. The deliberately-unmatched scalars (`ConfigVersion` and `Paused`, both worker-held; the four `Mcp*`/`Web*`, nested under `config.Mcp`/`config.Web`) are pinned as a set with written reasons, so a new view scalar either gets covered automatically or fails until someone justifies the exclusion. **"Scalar" means any value type or `string`, not `IsPrimitive || string`** - the filter the pin was first written with, and a hole in it, since `IsPrimitive` is false for `enum`, `decimal`, `DateTime`, `Guid` and `TimeSpan`: a future knob of any of those types would have been invisible to *both* tests, unprobed for propagation AND absent from the unmatched set meant to force a written reason, with the unprobeable-type throw unable to fire because it only sees what the filter already admitted. Widening it is what makes that throw load-bearing; enums are handled generically (two distinct declared members) and the widened filter admits exactly the same 13 properties today, so nothing about current coverage changed. Verified red first: reverting the one assignment fails `EveryMatchedScalar_IsPropagatedByApplyToConfig` naming the dropped knob.
 - **The viewer's two fleet timers can no longer stack store reads, and the Overview no longer double-fires one of them** ([#2907]) - `OnRefreshTimerTick` fired `RefreshServerStatusAsync` and `RefreshStoreSizeAsync` unawaited with **no in-flight guard**, while the two siblings in that same fan-out both had one (`_alertPollInFlight`, and `_refreshInFlight` plus its `_refreshRequested` replay). So a read that outlived the interval was simply joined by the next tick's. Three numbers make that expensive rather than untidy: the interval is `NocRefreshIntervalSeconds`, default 30 but clamped to a floor of **10**; the store connection string sets **`MaxPoolSize = 10`** deliberately ([#1566]), so read eleven waits `ConnectionTimeoutSeconds` for a slot and then throws a **connect** error that misattributes a slow store to the network; and the fan-out is not the "cheap pair of single-query reads" its comment claimed - `RefreshServerStatusAsync` awaits a second read through `UpdateCollectorHealthTextAsync`, `PollAlertsAsync` awaits a third through `UpdateServerSilencedAsync`, so one tick is **five** store reads before `RefreshVisibleAsync` starts, six while the AG tab is still hidden. **The two guards are deliberately different shapes**, because a dropped request is recoverable in one case and not the other: store size takes no scope, so the in-flight read paints the same field with the same answer and a plain drop loses nothing, whereas the freshness read is fetched before an add/edit/remove and applied to the fleet after it, so a server registered mid-read is absent from the dictionary and paints as never-collected - that caller gets the coalescing replay, and it is **opt-in per call site** so the periodic callers still drop, since replaying a timer would delete the interval's gap precisely when the store is slowest. `OnOverviewTimerTick` also stopped duplicating the fan-out; moving `OnRefreshTimerTick`'s fan-out below its tab early-return instead would have been a regression, silencing the alert poll and the store-size read entirely while a per-server tab is up, which is what "regardless of the visible tab" is there for. [#2901] bounded each of these reads at 15s and deliberately left the guarding alone; a deadline caps how long a **stacked** read holds a permit, which is a strictly smaller claim than not stacking. Pinned by walking the shipped source rather than by a unit test, because the defect was a seam and not a logic error - no path from a fan-out target to a `_dataService` read may exist without a single-flight guard on it, the guard matched by SHAPE so the three that predate the pin (`_refreshInFlight`, `_alertPollInFlight`, `AvailabilityGroupsTab`'s `_loading`, one level down) all satisfy it without an allowlist. Proven red five ways, including the one a naive check would have excused: the fix moves the freshness read into a helper, so deleting the guard leaves the guarded method itself touching no store at all.
 - **Deferred plan and text fetch debt is no longer paid by whichever collector visits the database next** ([#2902]) - the two carryover dictionaries [#2312]'s activity-driven fetch defers budget-capped ids into were keyed by `(server, database)` with no collector in the key, while the fetch that spends them is gated on a SKU flag - `CapturePlanXml` for plans, `FetchQueryTextSeparately` for text - rather than on a collector identity. **Five collectors reach that fetch off Azure** (`query_store`, `plan_correction`, `query_store_health`, `index_object_stats`, `database_scoped_config` - derived by invoking their own `RunsPerDatabase`/`BuildEnumerationQuery`, never listed by hand), and only `query_store` can ever CREATE debt there, because `ExtractPlanReferences` and its text twin return empty for any other collector's batch. So the other four arrived having referenced nothing, found `query_store`'s deferred ids under the key they happened to share, drained them, and had the fetch's wall clock recorded against their own `collection_log` row. **~346 s of Query Store fetch time over 38 h on one monitoring host was billed to `plan_correction`, `query_store_health` and `index_object_stats`** - parsed from the service log 2026-09-03 00:00 to 2026-09-04 14:12 UTC across 2,769 `query_store` runs, and identifiable because those runs carry a signature nothing else produces: **`probed = 0` with `ids > 0`**, a collector that fetched ids it never probed for. The store's own two-day per-collector aggregate says the same thing from the other side: `query_store_health` reads one cheap row per database per hour and averages 304 ms per run over 1,731 runs on 42 servers, with a **72,454 ms worst execution** in that window - a 238x tail on a collector whose own read cannot produce one. The key now carries the collector, so a collector drains only debt it created. The same widening covers the two sibling dictionaries built from the same key (`_observedPlanSize`'s learned plan size and backoff count, and `_textFetchFailures`), whose per-database counters those foreign visits were also resetting on their nothing-owed return path. **What this does NOT change.** It moves no work and saves none - the same ids are still fetched and the same bytes still ship, and `query_store`'s own measured cost rises by roughly what its siblings stop being charged. It is an attribution fix, and it is worth making because [#2847], [#2841] and [#2860] all read `collection_log` per collector, so this silently moved time between collectors and between cadence TIERS in the series those investigations reason from. It does change WHEN the debt is paid: it now waits for `query_store`'s next visit rather than the first of five, which at the default cadences is at most one 5-minute cycle later - `query_store` and `plan_correction` are both on the 5-minute tier while `query_store_health` is hourly, `index_object_stats` daily and `database_scoped_config` on-load only, so the drain opportunities that materially existed were the two 5-minute ones and the change roughly halves them. **There is no cap and no eviction on the debt** - ids leave only by landing, by proving target-side-gone on a provably uncut pass, or with the process - so halving that drain rate makes an existing capacity question VISIBLE rather than creating one: if `query_store`'s own candidate cap cannot cover what its probe finds missing, the backlog grows, and that was already true with `plan_correction`'s free passes masking it. [#2860]'s persisted phase columns are the instrument to watch it with. Pinned behaviourally rather than by shape - "the key has three fields" would pass for a key carrying a collection time, which fixes nothing and drops every deferred id on the floor - over the family derived from `CollectorCatalog.All`, with the dictionaries' own declared key type and the runner's own key builder as the artifacts under test, plus an IL read of the built assembly - on [#2898]'s shared decoder, per state machine, with the probe calls as positive controls checked first - proving both fetch bodies call that builder instead of re-inlining a tuple. Each of the five assertions was proven red by its own mutation of the shipped source: reverting the key (siblings drain the owner's ids), making it unique per call (the owner loses its own), dropping the database (two databases alias), re-inlining one key (only one body calls the builder), and a family member ceasing to enumerate (the non-vacuity floor). No schema change: the per-phase persistence rung is [#2860]'s and comes after this.
 - **The server-scoped watermark read no longer runs for a collector whose own cycle throws the answer away** ([#2797]) - `RunAsync` dispatches down exactly one of three paths, and two of them assign `context.Watermark` from a PER-DATABASE read before any query is built: the Azure per-database connection loop, and the enumerated per-item loop. A collector taking either path still executed the server-scoped `GetLastCollectedTimeAsync` on every cycle and read its answer with nothing. It is now skipped on exactly those two paths. On the current catalog that is `query_store` on every target, plus `deadlocks` / `blocked_process_report` / `long_query_completions` on Azure SQL DB only - **7 of 168 (definition, target) pairs**, verified by invoking the shipped predicate over `CollectorCatalog.All` in the built assembly rather than by reading the source. **The gate the issue itself proposed would have shipped a data-correctness bug, and that is the whole reason this is a DISPATCH-PATH predicate rather than a watermark-column one.** [#2797] suggested `PerDatabaseWatermarkColumn is not null`, the signal the per-item paths already key on. Four collectors declare both watermark columns and all four declare `RunsPerDatabase => target.IsAzureSqlDb`, but only `query_store` overrides `BuildEnumerationQuery`; the other three inherit `CollectorDefinitionBase`'s `=> null`. So OFF Azure - the entire on-prem and RDS fleet - three of the four fall through to the plain server-scoped path and genuinely CONSUME the value, and gating on the column alone would have handed them a null watermark every cycle and made them re-collect their whole fallback window forever: [#2795]'s defect, arrived at from the other side. The fleet said the same thing before the change, because V108's phase columns are written only on the server-scoped path and so identify dispatch directly - over 90 minutes on 42 servers `blocked_process_report` populated `sql_open_ms` on 2,122 of 2,122 runs and `deadlocks` on 632 of 632, while `query_store` populated it on **0 of 633**. **What this is worth, with its provenance, because the figure is easy to misattribute.** The read removed is `MAX(last_execution_time) ... WHERE server_id = $1 AND collection_time > $2` over `collect.query_store_stats`, the store's largest table, once per `query_store` cycle per server. The only direct measurement of that read is [#2796]'s, taken as it was being bounded: **2.9-5.9 s** warm-to-cold against a 62.5 GB table (40.7 s and 50.6 s before the bound). It cannot be confirmed from the fleet today, and that is a stated gap rather than an omission - `ServerWatermarkMs` reaches the phase log only under `ServerPhasesMeasured`, which the enumerated path leaves false, and V108's `watermark_ms` column is persisted on that same branch, so for `query_store` specifically this read's cost is recorded in neither the log nor the store ([#2860]). The `wm:` field that IS populated for `query_store` in the phase log belongs to the per-item line and measures the PER-DATABASE read (p50 41 ms over 822 samples): a different read, legitimate incremental work, and untouched here - so reading it as the server-scoped cost understates this change by whatever the ratio is between a whole-server `MAX` over the three-hour read-floor window and one database's. The `deadlocks` and `blocked_process_report` server-scoped reads visible in that same log (p50 9 ms and 5 ms) are also legitimate and deliberately stay. **Pinned over the FAMILY derived from `CollectorCatalog.All`**, not by naming collectors, the way [#2796]'s pins are derived from the runner's method family: a fifth collector that declares both columns and enumerates is covered the day it appears, and one that stops enumerating is un-covered automatically. The load-bearing assertion is the NEGATIVE one - that the three server-scoped members still get a non-null watermark off Azure - and it was verified red against the issue's own proposed gate before being left green. The enumeration half of the predicate is derived by CALLING the definition's `BuildEnumerationQuery` rather than mirrored into a second declared flag, so the two cannot drift apart; the probe context that makes that answerable before the cycle's own context exists (`HasCollectedBefore` is computed FROM the gated read, and it, `NumericWatermark` and `State` are init-only) is sound only while null-ness depends on `Target` alone, which a further pin asserts across **5,376** (definition, target, context) combinations instead of assuming. Skipping the read also had to gate `HasCollectedBefore`, whose branch keys off `watermark is null` and would otherwise have fired its own store query on precisely the cycles the change exists to make cheaper.
@@ -3244,6 +3245,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#2890]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/2890
 [#2882]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2882
 [#2888]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2888
+[#2917]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2917
+[#2918]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2918
 [#2907]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2907
 [#2901]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/2901
 [#2902]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2902