Fixes #2923

CHANGELOG.md

@@ -118,6 +118,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
+- **`ViewerFleetTimerGuardTests` no longer passes when the fleet-timer fan-out moves BELOW the tab early-return, which is the regression [#2907] named** ([#2923]) - `NoStoreRead_IsFiredByBothFleetTimers` cuts `OnRefreshTimerTick` at its first `return;` and intersects only the region ABOVE it with `OnOverviewTimerTick`, so relocating the fan-out below that return empties the region, empties the intersection, and satisfies the assertion BECAUSE the regression happened; measured, that mutation leaves all four of that class's facts green, and the `firstReturn.Success` staleness guard cannot see it because a relocation leaves the `return;` exactly where it was. [#2907] warned about this move in the same breath as the duplication that pin does catch - it "would stop the alert poll and the store-size read entirely while the Overview or a per-server tab is up" - and the Overview is the tab that ships selected. `ViewerFleetTimerFanOutPositionTests` now asserts the ORDERING: every fire-and-forget in the tick has a call above the first `return;`, the awaited `RefreshVisibleAsync` stays below it, and the pinned set is DERIVED from the tick with the three names [#2907] argues about asserted to still be in it, so a rename is loud rather than vacuously green. Read over `CSharpSourceWalker`'s output ([#2913]) rather than a sixth private copy of the walk. **A count-based control cannot prove this red** - a relocation leaves every occurrence count invariant, 1 to 1 - so the control is the offset comparison itself, carried in CI by a third fact that runs the check twice over one relocated fixture: detected through the walker, missed over the same text raw, where the fixture's own comment prose supplies the above-the-return hit. Nine mutations, each verified applied before it was trusted (by offset for the relocations, by count for the rest) and compiled first, each failing a different assertion - the relocation now fails the new position fact with all four old facts still green, and those four each still go red on their own mutation. Same PR folds `BraceBalanced` into `CSharpSourceWalker` and deletes both private copies - the new pin's and `ViewerFleetTimerGuardTests`' - because brace-matching is only correct over the walker's output: braces in prose and in literals are exactly what unbalance it, which is already why blanking a hole's DELIMITERS is part of `StripCommentsAndStrings`' contract. The two copies had already drifted textually, the newer one having fused the decrement into its condition, and they were proven equivalent at **every** offset of all **190** viewer `.cs` files (**2,836,304** positions, zero differences) before one was picked, with the differential harness positive-controlled against a third form that omits the decrement; the older expanded form was kept because it is the one on `dev` and matches `StatementSpanFrom`'s style in the file it moves into. `BodyAfterSignature` deliberately stays put - one class, two call sites, no other caller in the repo, and a contract specific to that pin's call-graph walk (it returns an expression-bodied member's expression WITHOUT braces so the walk can follow `RefreshAgAsync => LoadAsync();`), so moving it would grow the shared surface without removing a duplicate. The move is shown behaviour-preserving THROUGH the pins rather than merely still-green: the relocation mutation's failure message is byte-identical across it (offsets 2344 / 2773 / 2814 / 2852), so the shared helper extracts the same body both copies did, and all seven mutations still fail the same assertions with the test count unmoved at 7.
 - **The source-walking idiom behind five test pins no longer blanks interpolated-string holes, so a call written inside an interpolation is visible to the scans built on it** ([#2913]) - `AlertPassCommandTimeoutTests`, `AnalysisPassCommandTimeoutTests`, `StorageCommandTimeoutTests`, `ViewerCommandTimeoutTests` and `ViewerFleetTimerGuardTests` each carried a private copy of the same walk (three copies of `StripCommentsAndStrings`, three of `StatementSpanFrom`, **five** of `SkipVerbatimString`/`SkipRegularString`), and every one blanked the ENTIRE span of a string literal, holes included - so `Log($"count={Foo()}")` had no `Foo()` in it as far as any scan could tell. **For interpolation this was latent and stayed latent: no pin's verdict changes.** Every one of the **362** command-construction sites the three timeout pins judge (193 viewer, 124 storage, 45 alert-pass) comes back with a byte-identical statement span and the same timed/untimed verdict, the method-group hand-off sweep still finds zero on all four scanned sets, and the fleet-timer pin's four assertions still pass - it is worth fixing because the failure direction is the dangerous one, not because it was lying. **Two of the gaps were not latent.** Raw string literals did not exist for the old walk: it read the opening `"""` as an empty `""` plus the start of another string, so the delimiter itself desynchronised the parse - and the viewer project alone holds raw strings in **36** files, `$"""` in **7**, and one `$$"""` (`ViewerDataService.Waits.cs`, whose wait-trend SQL interpolates `{{typeParams}}`). A `char` literal was plain code, which makes `value.Contains('"')` - a real line in `ViewerServerTab.ChartContextMenu.cs`, inside the directory both viewer pins glob recursively - open a string that runs to the next `"` anywhere in the file. **Measured over the 269 files the five pins read: the old walk leaves 2 of them bracket-unbalanced after stripping** (`ViewerServerTab.ChartContextMenu.cs` at braces -1 / parens +2, `StoreConnectionSelfTest.cs` at braces +3), which is not a curiosity when the fleet-timer pin extracts method bodies by matching braces in that same output; the new walk leaves none. Across the viewer set it puts **12,438** characters of hole contents into the code stream and takes **74,111** characters of literal text out of it (10,076 in `ViewerDataService.QueryStore.cs` alone), which for that pin's call graph is **25 call edges it could not previously see** and **235 phantom ones** it was following - `AND`, `ANY`, `COALESCE`, `date_trunc` and friends, SQL keywords leaking out of desynchronised raw strings. `_dataService` read sites are unchanged at 308 either way. All five now share one `CSharpSourceWalker` that classifies each character ONCE - hole contents are code, hole delimiters and format specifiers are not, `{{`/`}}` are escapes, `$$` needs two braces to open a hole, and `"""`, `@"..."`, `$@"`/`@$"` and `char` literals are all recognised - with `StripCommentsAndStrings` and `StatementSpanFrom` built on that one mask, so the two walks can no longer disagree about what a literal is. Widening the balance sweep from those 269 files to all **2,087** `.cs` files in the repo found one more, in the new walker rather than the old: a raw string literal cannot carry the `@` prefix, so a quote RUN after `@` is an escaped quote and not a delimiter - `@"\[""" + Regex.Escape(name) + @"""\]..."` in `Lite.Tests/CrossAppPresetValuePinTests.cs`. It left exactly two of the 2,087 unbalanced, both that shape, and **none** of the 269 the pins read, so it is pinned as an arranged fixture rather than by keeping a corpus assertion over code nobody scans. Repo-wide the old walk leaves **43** files unbalanced and the new one none. The `$$` brace-run rule is the compiler's, checked rather than assumed: `$"""{{"""` does not compile at all (CS9006), which is why the doubled-brace escape is pinned on the non-raw forms where it is legal. **The copies had already drifted** - `ViewerFleetTimerGuardTests`' regular-string skip had grown a newline stop the other four lacked, so one of five had hardening the rest did not, which is the other half of the argument for having one. The walker carries its own witnesses (**15** new tests, one per construct, each stated as a difference against the pre-#2913 walk transcribed alongside them rather than against a comment claiming it was wrong, and every `DoesNotContain` confirming the FIXTURE holds the needle before asserting the stripped output does not): re-blanking holes fails seven of them, forgetting raw strings fails two, treating `{{` as a hole fails exactly one, dropping `char` literals fails three, treating a format specifier as code fails one, reading a quote run after `@` as a raw delimiter fails one, and breaking newline preservation fails one - **seven** mutations, seven distinct failure sets, each one's anchor count asserted before the run, with all five consolidated pins staying green under every one of them, which is the latency claim above restated as a test.
 - **The compose `statement_timeout` now reaches the live roles on a reload, instead of only on a service restart** ([#2918]) - it was the **only** `config_service` knob whose change an operator had to restart the service to deliver, and the reason is that it does not live in a query: it lives on the ROLES, as `ALTER ROLE viewer/mcp SET statement_timeout`, written once per process by startup provisioning. [#2917] fixed the held config's truthfulness and deliberately changed nothing here, so a reload still observed the new value and left the roles on whatever the last start wrote. **That is worst in the one situation the backstop exists for** - a runaway aggregation pinning a raw, no-rollup store - where "restart the collector" is a bad answer. `ReloadFromStoreAsync` now re-asserts the two statements, **only on a real change**: a `config_version` bump fires on any `config_service` or schedule write, and this is a catalog write, so re-asserting unconditionally would charge every unrelated reload for a knob nobody touched. There was no privilege obstacle to find - the service's store connection is the owner (`PostgresConfig.ConnectAs` is the *Viewer's* role selector and is documented as ignored by the service). Gated exactly as startup provisioning is (managed + Windows), because that is where these roles are CREATED; a BYO store provisions them out-of-band through `tools/provision-roles.sql` and names them itself, so `ALTER ROLE viewer` there would be guessing at an identity we do not own. **A role `SET` only takes on that role's NEXT session**, which is what makes it cheap and safe - it cannot disturb a query already running, and an already-connected viewer keeps the old ceiling until it reconnects, so lowering the ceiling bounds the next runaway rather than the one in flight. **The load-bearing invariant is no-drift**, because two paths now write the same pair of statements and a disagreement would fail nothing - both would run, and the live ceiling would depend on which touched the roles last. So there is exactly ONE renderer (`DarlingManagedRoles.BuildComposeStatementTimeoutSql`), the provisioning batch embeds its output, and the pin splices the **shipped** renderer into the assertion rather than a retyped copy - plus a count check, since an embed that left the old inline copy behind would still satisfy `Contains` while writing the ceiling twice. The reload decision is a pure static predicate rather than a condition buried in the worker loop, so **when to pay a catalog write** - the whole design - is testable without a host, including that the baseline advances only on SUCCESS: recording the attempt would make one transient failure permanent, leaving the roles on the old ceiling forever while the held config claimed otherwise, which is [#2917]'s split reintroduced one layer down. **Review caught a first-run ordering trap in the first cut of this**, worth recording because it is the same defect one level down: role provisioning runs BEFORE `SeedIfEmptyAsync`, so on a brand-new managed store there is no `config_service` row to read and the roles get the 15 s default, and only then does the seed insert `darling.json`'s value. Seeding the reload baseline from the post-seed store view therefore recorded a value the roles never received - and because the gate fires only on a difference, that mismatch was PERMANENT for the life of the process, defeating no-restart delivery for precisely the operator who set the knob up front. `EnsureProvisionedAsync` now RETURNS the clamped value it wrote and the baseline is seeded from that, so it means what its name says; it stays at the -1 sentinel if provisioning threw, so the first reload converges rather than trusting a write that did not land. The clamp itself is now the one in `StoreConfigProvider` at all three sites rather than a re-derived formula - clamping the SQL text's bounds by a second copy of the rule, in a change whose whole point is removing that hazard from the SQL text, was not a coherent place to stop.
 - **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.
@@ -3292,3 +3293,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#2760]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2760
 [#2847]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2847
 [#2913]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2913
+[#2923]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2923