diff --git a/CHANGELOG.md b/CHANGELOG.md index b8f24465a6..ce5f6ddf24 100644 --- a/CHANGELOG.md +++ b/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 `win-x64` 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 diff --git a/Darling/Darling.Tests/CSharpSourceWalker.cs b/Darling/Darling.Tests/CSharpSourceWalker.cs index 42336c9f9a..2a8a59f4f7 100644 --- a/Darling/Darling.Tests/CSharpSourceWalker.cs +++ b/Darling/Darling.Tests/CSharpSourceWalker.cs @@ -159,6 +159,44 @@ internal static string StatementSpanFrom(string text, int start, int statements) return text[start..]; } + /// + /// The brace-balanced block starting at , that brace included — or the + /// rest of when the braces never balance, so a caller gets a truncated body + /// rather than an exception on malformed input. + /// + /// It lives here because it is only correct over the output of + /// : braces in prose and in literals are exactly what would + /// unbalance it, which is why blanking a hole's DELIMITERS is part of that method's contract. Two + /// viewer pins had grown their own copy by #2923 — behaviourally identical over 2,836,304 offsets of + /// the viewer project but already textually apart, one having fused the decrement into its condition, + /// which is how the five walk copies started too. + /// + internal static string BraceBalanced(string text, int open) + { + ArgumentNullException.ThrowIfNull(text); + + var depth = 0; + + for (var i = open; i < text.Length; i++) + { + if (text[i] == '{') + { + depth++; + } + else if (text[i] == '}') + { + depth--; + + if (depth == 0) + { + return text[open..(i + 1)]; + } + } + } + + return text[open..]; + } + /// /// Marks code from onwards. When is supplied we are inside /// that literal's interpolation hole, and the scan RETURNS at the hole's closing brace run or at its diff --git a/Darling/Darling.Tests/ViewerFleetTimerFanOutPositionTests.cs b/Darling/Darling.Tests/ViewerFleetTimerFanOutPositionTests.cs new file mode 100644 index 0000000000..2978feae94 --- /dev/null +++ b/Darling/Darling.Tests/ViewerFleetTimerFanOutPositionTests.cs @@ -0,0 +1,342 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Text.RegularExpressions; +using Xunit; + +namespace Darling.Tests; + +/// +/// OnRefreshTimerTick's freshness fan-out has to sit ABOVE its tab early-return. That is a property +/// of WHERE the calls are written rather than of which calls exist, and it is the one property +/// cannot see (#2923). +/// +/// The hole this closes. +/// cuts the tick at its first +/// return; and intersects only the region ABOVE it with OnOverviewTimerTick. Move the fan-out +/// below that return and the region empties, the intersection empties with it, and the assertion is +/// satisfied BECAUSE the regression happened. Measured by mutation: relocating the three unconditional +/// calls below the early-return leaves all four of that class's facts GREEN. Its staleness guard — +/// Assert.True(firstReturn.Success, …) — fires only when the early-return DISAPPEARS, and a +/// relocation leaves the return; exactly where it was. +/// +/// The relocation is a named regression, not a tidy-up. #2907, in the same breath as the +/// duplication that pin does catch: moving the fan-out below the early-return "would stop the alert poll +/// and the store-size read entirely while the Overview or a per-server tab is up, which is the opposite of +/// what 'regardless of the visible tab' is there for." The Overview is the tab that ships selected, so the +/// fan-out would be dead for most operators most of the time with every existing assertion green. +/// +/// The pinned set is derived, with a name list only as its staleness guard. Every +/// fire-and-forget in the tick is checked, so a fan-out call added later is covered without anyone +/// remembering this file; the three names #2907 argues about are asserted to still BE in that set, so a +/// rename is loud rather than vacuously green. One corollary is deliberate: a new _ = X() written +/// BELOW the early-return fails this pin. Everything below that return is tab-conditional by construction, +/// so a fire-and-forget that genuinely belongs there wants a sentence here saying why. +/// +/// Offsets, because a count cannot control a relocation. Moving code leaves every occurrence +/// count invariant — one _ = RefreshServerStatusAsync(); before, one after — so an anchor tally +/// reports the mutation as applied whether it applied or not, and reports this pin as red-capable when it +/// is not. The comparison itself is the only control, which is why every message here carries the offsets. +/// Measured on the shipped source as of this pin, and DESCRIPTIVE — nothing below asserts an absolute +/// value, only an ordering: with the body taken from its opening brace, the first return; is at +/// 2455, the four fan-out calls at 1041, 1082, 1322 and 1755, and the visible-tab refresh at 2886. +/// (#2923 cites 2454 for the return, which is the same place counted from just inside the brace, and 2881 +/// for _ = RefreshServerStatusAsync();, which that call has at no convention — it lands on the +/// awaited visible-tab refresh at the bottom of the method. The fan-out is 1,414 characters ABOVE the +/// return, so the direction the issue concludes is right even though the number it quotes reads as +/// below.) +/// +/// The walk is (#2913/#2925), not a sixth private copy — +/// brace-matching its output to find a method body is what that class's own summary says the output is for. +/// Worth being exact about what it buys HERE, because it is less than #2923 claims. The tick's +/// comment-only mentions are ONE each of RefreshServerStatusAsync, PollAlertsAsync and +/// RefreshVisibleAsync, none of RefreshStoreSizeAsync and none of return — the +/// issue's 2/1/2/2/2 are the totals including the code. And none of them is call-shaped, so raw and +/// stripped agree on every call offset in this tick and stripping does not change today's answer. What it +/// buys is the NEXT comment: those three mentions sit above the early-return, at offsets 328, 472 and 622, +/// so one comment written PollAlertsAsync() instead of PollAlertsAsync — or any widening of +/// the match to a bare name — turns prose into an above-the-return hit and this pin green on the +/// regression. +/// runs that case both +/// ways, so the claim is a test rather than a sentence. +/// +public sealed class ViewerFleetTimerFanOutPositionTests +{ + /// The tick whose fan-out position is the subject; the Overview tick has no early-return to + /// split on and is 's side of the pair. + private const string RefreshTick = "OnRefreshTimerTick"; + + /// The tab-conditional half: awaited, and the one call that must sit BELOW the return. + private const string VisibleRefresh = "RefreshVisibleAsync"; + + /// + /// The three calls #2907 argues about by name. Not the pinned set — that is derived — but a staleness + /// guard on it, so a rename cannot leave behind a derived set that is trivially satisfied. + /// + private static readonly string[] s_namedFanOut = + [ + "PollAlertsAsync", + "RefreshServerStatusAsync", + "RefreshStoreSizeAsync", + ]; + + /// A fire-and-forget call — the shape that makes a tick a fan-out. + private static readonly Regex s_fireAndForget = new( + @"_\s*=\s*(\w+)\s*\(", + RegexOptions.Compiled | RegexOptions.CultureInvariant); + + /// The same expression splits on, deliberately: the + /// two pins have to agree on where the unconditional region ends, or this one is green about a boundary + /// the other one is not using. + private static readonly Regex s_earlyReturn = new( + @"\breturn\s*;", + RegexOptions.Compiled | RegexOptions.CultureInvariant); + + [Fact] + public void TheUnconditionalFanOut_IsFiredAboveTheTabEarlyReturn() + { + var body = StrippedTickBody(RefreshTick); + var split = EarlyReturnOffset(body); + var fired = FiredNames(body); + + /* Floor first: an empty set satisfies the position assertion vacuously, which is the shape of + failure #2923 is about. Four when this landed — status, store size, alert poll, AG probe. */ + Assert.True( + fired.Count >= 4, + $"only {fired.Count} fire-and-forget call(s) parsed out of {RefreshTick} — the sweep is not " + + "reading MainWindow, so the position assertion below would hold over nothing"); + + var renamed = s_namedFanOut.Where(n => !fired.Contains(n)).ToList(); + + Assert.True( + renamed.Count == 0, + $"{RefreshTick} no longer fires {string.Join(", ", renamed)}; this pin's name list is stale, and " + + "a stale list is what would let the derived check hold while the calls #2907 argues about are " + + "gone"); + + var offenders = FanOutNotAbove(body, split); + + Assert.True( + offenders.Count == 0, + $"{RefreshTick}'s first return; is at body offset {split} and these fan-out calls are no longer " + + "above it, so they stop firing entirely while the Overview or a per-server tab is up — the " + + $"regression #2907 named and #2923 found unguarded: {string.Join("; ", offenders)}"); + } + + /// + /// The other half of the ordering, and the reason the sibling pin's split means anything: the + /// visible-tab refresh has to stay BELOW the early-return. Above it, this timer and the Overview's own + /// would refresh the same grid every cycle at the same interval — which is the double-refresh the + /// return exists to prevent, and exactly the narrow claim #2907's comment makes about the GRID while + /// the freshness reads above the return are deliberately unconditional. + /// + [Fact] + public void TheVisibleTabRefresh_IsAwaitedBelowTheTabEarlyReturn() + { + var body = StrippedTickBody(RefreshTick); + var split = EarlyReturnOffset(body); + var offsets = CallOffsets(body, VisibleRefresh); + + Assert.True( + offsets.Count > 0, + $"{RefreshTick} no longer calls {VisibleRefresh}, so this pin's ordering has nothing left to " + + "hold — the tab-conditional half of the tick has moved or gone"); + + Assert.True( + offsets.All(o => o > split), + $"{VisibleRefresh} is called at offset(s) {string.Join(",", offsets)} in {RefreshTick}, at or " + + $"above its first return; ({split}) — so it runs on the tabs that return exists to skip and " + + "the Overview grid is refreshed twice a cycle by two timers at one interval"); + } + + /// + /// The control, in CI rather than in a commit message: the position check has to actually REPORT a + /// fan-out relocated below the early-return, and it has to be the walker that lets it. A tally cannot + /// control a relocation, because a relocation leaves every tally where it was — so the control is the + /// direction of the comparison, run twice over one fixture. + /// + /// The fixture's comment names its calls in call shape, which is one keystroke from what the + /// shipped comments already say. Over raw text those mentions supply an above-the-return hit for every + /// relocated call and the check reports nothing; over the walker's output they are blanked and it + /// reports all three. A failure of the second assertion is a statement about this fixture — it has + /// stopped demonstrating the miss — and not about MainWindow. + /// + [Fact] + public void TheAboveTheReturnCheck_SeesARelocatedFanOut_OnlyThroughTheWalker() + { + var stripped = CSharpSourceWalker.StripCommentsAndStrings(RelocatedFanOutFixture); + var seen = FanOutNotAbove(stripped, EarlyReturnOffset(stripped)); + + Assert.True( + seen.Count == s_namedFanOut.Length, + "the position check does not report a fan-out relocated below the early-return, so " + + $"{nameof(TheUnconditionalFanOut_IsFiredAboveTheTabEarlyReturn)} cannot fail in the one " + + $"direction #2923 exists for: [{string.Join("; ", seen)}]"); + + var missed = FanOutNotAbove(RelocatedFanOutFixture, EarlyReturnOffset(RelocatedFanOutFixture)); + + Assert.True( + missed.Count == 0, + "the raw-text half of this control no longer misses the relocation, so the fixture has stopped " + + $"showing what the walker is for here: [{string.Join("; ", missed)}]"); + } + + /// + /// OnRefreshTimerTick's body as the #2923 mutation leaves it — fan-out below the early-return — + /// carrying comment prose above that return which names every relocated call in call shape. + /// + private const string RelocatedFanOutFixture = """ + { + /* Fires the freshness fan-out every cycle regardless of the visible tab: _ = PollAlertsAsync(); + and _ = RefreshServerStatusAsync(); and _ = RefreshStoreSizeAsync(); — prose that says the + fan-out is up here while the code doing it is not. */ + if (ReferenceEquals(MainTabs.SelectedItem, OverviewTab)) + { + return; + } + + _ = RefreshServerStatusAsync(); + _ = RefreshStoreSizeAsync(); + _ = PollAlertsAsync(); + + await RefreshVisibleAsync(); + } + """; + + // ── The position check, shared by the pin and its control ───────────────────────────── + + /// + /// Every fire-and-forget target in a tick body with no call above , reported + /// with its offsets because those are the one thing a relocation cannot leave invariant. Empty is the + /// passing state. + /// + /// ANY call shape counts as "above", not only the fire-and-forget the name was collected by: + /// this pin is about WHERE a call is, and whether it is awaited up there is the single-flight question + /// already owns. + /// + private static List FanOutNotAbove(string body, int split) + { + var offenders = new List(); + + foreach (var name in FiredNames(body).OrderBy(n => n, StringComparer.Ordinal)) + { + var offsets = CallOffsets(body, name); + + if (offsets.Any(o => o < split)) + { + continue; + } + + offenders.Add($"{name} called at offset(s) {string.Join(",", offsets)}, all below {split}"); + } + + return offenders; + } + + /// The distinct names a body fires unawaited. + private static HashSet FiredNames(string body) => + s_fireAndForget.Matches(body) + .Select(m => m.Groups[1].Value) + .ToHashSet(StringComparer.Ordinal); + + /// Every offset at which is invoked, whatever syntax invoked it. + private static List CallOffsets(string body, string name) => + Regex.Matches(body, @"\b" + Regex.Escape(name) + @"\s*\(") + .Select(m => m.Index) + .ToList(); + + /// + /// Offset of the tick's first bail-out return; — the boundary between what runs regardless of + /// the visible tab and what does not. Fails rather than returning a sentinel: with no return there is + /// no position left to pin, and a check that silently degrades to "no boundary, nothing below it" is + /// the pass-for-the-wrong-reason this pin exists to remove. + /// + private static int EarlyReturnOffset(string body) + { + var match = s_earlyReturn.Match(body); + + Assert.True( + match.Success, + $"{RefreshTick} has no bail-out return;, so there is no boundary for the fan-out to sit above — " + + "and ViewerFleetTimerGuardTests' region split is stale in the same breath"); + + return match.Index; + } + + // ── Source access ───────────────────────────────────────────────────────────────────── + + /// + /// One named method's brace-balanced body, cut from the viewer shell AFTER + /// has blanked its comments and literals — so a brace in prose cannot + /// unbalance the cut and a mention in prose cannot be read as a call. The walk preserves length, so an + /// offset into this body is the same offset the raw file has. + /// + private static string StrippedTickBody(string name) + { + var code = CSharpSourceWalker.StripCommentsAndStrings(File.ReadAllText(ShellPath())); + var signature = Regex.Match(code, @"\bvoid\s+" + Regex.Escape(name) + @"\s*\("); + + Assert.True(signature.Success, $"{name} is no longer declared in MainWindow.xaml.cs"); + + /* Past the parameter list before looking for the body brace: a parameter default can carry one. */ + var i = signature.Index + signature.Length - 1; + var depth = 0; + + for (; i < code.Length; i++) + { + if (code[i] == '(') + { + depth++; + } + else if (code[i] == ')' && --depth == 0) + { + break; + } + } + + var open = code.IndexOf('{', i); + + Assert.True(open >= 0, $"{name} has no body in MainWindow.xaml.cs"); + + return CSharpSourceWalker.BraceBalanced(code, open); + } + + private static string ShellPath() + { + var path = Path.Combine( + RepoRoot(), + "Darling", + "PerformanceMonitor.Darling.Viewer", + "MainWindow.xaml.cs"); + + Assert.True(File.Exists(path), $"the viewer shell is not where this pin looks: {path}"); + + return path; + } + + private static string RepoRoot([CallerFilePath] string thisFile = "") + { + var dir = Path.GetDirectoryName(thisFile)!; + + while (dir is not null + && !File.Exists(Path.Combine(dir, "PerformanceMonitor.sln")) + && !Directory.Exists(Path.Combine(dir, ".git"))) + { + dir = Path.GetDirectoryName(dir); + } + + Assert.NotNull(dir); + + return dir!; + } +} diff --git a/Darling/Darling.Tests/ViewerFleetTimerGuardTests.cs b/Darling/Darling.Tests/ViewerFleetTimerGuardTests.cs index abb5d3149a..e1d06babc1 100644 --- a/Darling/Darling.Tests/ViewerFleetTimerGuardTests.cs +++ b/Darling/Darling.Tests/ViewerFleetTimerGuardTests.cs @@ -495,7 +495,7 @@ private static IEnumerable FinallyBlocks(string body) continue; } - yield return BraceBalanced(body, open); + yield return CSharpSourceWalker.BraceBalanced(body, open); } } @@ -614,7 +614,7 @@ private static Dictionary> ViewerMethods() if (code[j] == '{') { - return BraceBalanced(code, j); + return CSharpSourceWalker.BraceBalanced(code, j); } if (code[j] == '=' && j + 1 < code.Length && code[j + 1] == '>') @@ -627,30 +627,6 @@ private static Dictionary> ViewerMethods() return null; /* `;` — an abstract/partial/interface declaration with no body here. */ } - private static string BraceBalanced(string code, int open) - { - var depth = 0; - - for (var i = open; i < code.Length; i++) - { - if (code[i] == '{') - { - depth++; - } - else if (code[i] == '}') - { - depth--; - - if (depth == 0) - { - return code[open..(i + 1)]; - } - } - } - - return code[open..]; - } - private static string Body(IReadOnlyDictionary> methods, string name) { Assert.True(methods.TryGetValue(name, out var declarations), $"{name} is no longer declared in the viewer project"); @@ -726,12 +702,13 @@ private static bool IsBuildOutput(string projectDir, string path) } /* The literal- and comment-aware walk this pin used to carry lives in CSharpSourceWalker as of - #2913. It was one of five private copies, and all five blanked an interpolated string's HOLES along - with the literal text around them, so a call written inside an interpolation was invisible to every - scan built on them — including this one, whose whole job is to follow call edges. This copy had - also drifted from the other four (its regular-string skip had grown a newline stop the others - lacked), which is the other half of the argument for having one. CSharpSourceWalkerTests carries - the witnesses. */ + #2913, and its brace-matching as of #2923 — the walk's output is what makes brace-matching safe, + so the two belong in one place. It was one of five private copies, and all five blanked an + interpolated string's HOLES along with the literal text around them, so a call written inside an + interpolation was invisible to every scan built on them — including this one, whose whole job is + to follow call edges. This copy had also drifted from the other four (its regular-string skip had + grown a newline stop the others lacked), which is the other half of the argument for having one. + CSharpSourceWalkerTests carries the witnesses. */ private static string RepoRoot([CallerFilePath] string thisFile = "") {