Merge origin/dev; CHANGELOG entry moved to the coordinating change

Author: erikdarlingdata

CHANGELOG.md

@@ -118,9 +118,6 @@ 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
-- **The collection-sweep deadline's floor no longer credits `CommandTimeout` with bounding a phase it cannot reach** ([#2874]) - `ServiceCommandDeadlines.CollectionSweepSeconds` derived its floor as the **1.53 s** measured store write PLUS the **673-893 ms** of store connection acquisition [#2819] measured, reaching ~2.4 s, and the collection-sweep pin's own failure message said the same thing. Acquisition does not belong in a `CommandTimeout` floor: `CommandTimeout` starts when the command EXECUTES on an already-open connection, while the connect phase is tracked by the connection string's `Timeout`. That is measured rather than argued - with `Timeout=2` a connect failure lands at **2.0 s** whether `CommandTimeout` is 1 or 60. **The value does not move and was not re-derived**: the floor drops from ~2.4 s to the 1.53 s actually observed, ~3.1 s once projected to `max_concurrent_sweeps` = 8, and 10 s clears that by ~3.2x - so this is the justification becoming true, not a number changing. The doc comment now states WHY the connect phase is out of scope, with that measurement as the evidence, so it does not get folded back in. The **673-893 ms is still correct where it actually binds** - pool pressure, in `DarlingManagedPostgres`, `DarlingCollectorRunner` and `FetchStoreConnectionBorrowTests` - and those three are deliberately untouched.
-- **A comment that quotes the deadline can no longer certify an untimed collection-sweep site, and the fixture that was supposed to witness that no longer passes for the wrong reason** ([#2874]) - `CollectionSweepCommandTimeoutTests` matched the CONSTRUCTION over comment- and literal-stripped source, so a construction named in prose could not be counted as a site, and then judged the DEADLINE over RAW source. One half stripped, the other not, inside the same method. A `/* */` block in the two-statement window spelling `command.CommandTimeout = ...` therefore made an untimed site read clean - at a line where no edit could ever fix it - and the same asymmetry sat on the COPY sweep's `importer.Timeout = ...`. Both value regexes now read the stripped, character-aligned span. **Stripping the value span is not unconditionally correct for this family of pins**: one whose value regex has to see inside a string LITERAL would be broken by it, so the target of each pattern was checked rather than the change swept in - `CommandTimeout\s*=` and `\.Timeout\s*=` both match an assignment in code, which is why it is right here. **The fixture correction is the substantive half.** The case that read as the comment-immunity witness wrote its prose as `Timeout =`, with no leading dot, while the scanner requires `\.Timeout\s*=` - so it passed on the missing dot rather than on comments being ignored, and the COPY sweep looked guarded against this while it was not. Rewritten the way anyone documenting an absent deadline actually writes it (`importer.Timeout = TimeSpan.FromSeconds(n) is set by the caller`), it reads TIMED over the raw span and correctly UNTIMED over the stripped one, which is the evidence it now witnesses the property it names. The command half had no witness at all; `TheDeadlineScanner_ReadsCodeNotProse` is it, routed through the same helper the census uses so a control cannot drift from the code it controls, and covering the literal direction as well as the comment one.
-- **The collection-sweep census can now see a construction that carries its namespace, and it records which of its sites address the store rather than a monitored target** ([#2874]) - [#2928]'s `CollectionSweepCommandTimeoutTests` matched `new NpgsqlCommand(` and `.CreateCommand(`, neither of which matches `new Npgsql.NpgsqlCommand(` - so a construction written with its namespace on it was counted zero times and had its deadline examined never, inside twelve members whose correctness that pin asserts. **The blind spot has no defect behind it, and the census did not move.** The qualified form occurs exactly once in the repo - `DarlingWorker.ReadPgStatementTextAsync`'s `pg_stat_statements` text fetch - and that method is not one of the twelve: it is called by the enlisted `TryRefreshPgStatementTextAsync` but is a sibling helper, so its construction never sat inside a scanned body. Re-scanning all twelve members under the old pattern and the new one returns **13** either way, which is what `ExpectedSweepCommandSites` is re-derived to rather than adjusted: ten `new NpgsqlCommand(sql, connection)` sites on a connection borrowed from the store pool, and three `_postgres.CreateCommand(` on the data source itself. **What the count cannot include is measured rather than assumed**: a target-typed `new(...)` puts the type in the member's return type rather than at the construction, so no regex anchored there can reach it (#2943 reports the shape and calls it unguardable that way, which is the right call) - target-typed `new(` occurs **zero** times inside these twelve member bodies, and none of the 39 `=> new(` sites elsewhere in `.Service` returns a command, a connection or an importer. The constant's name is now documented as "constructions matching the shapes the pattern can see" rather than "constructions", so the next reader does not take the count as complete. The shape is closed anyway, because declining to close a shape a census demonstrably cannot see is this issue's own mistake in miniature. **The text fetch is classified as a MONITORED-TARGET read and deliberately left alone.** Its connection is built from `runtime.ConnectionString`, and its `CommandTimeout = 60` is not a bespoke bound: `MonitoredServerConnection.BuildConnectionString` puts `CommandTimeout = 60` on every monitored-PostgreSQL connection string and Npgsql initialises a command's deadline from its connection's, so the site RESTATES the target-wide value it would have inherited. Nor is it a lone literal: `CommandTimeout = 60` appears **26** times in this project - 23 target commands in `DarlingXeSessions`, both of `MonitoredServerConnection`'s connection-string builders, and this site. Folding it into the sweep regime would have replaced that with the store's 10 s: a **6x tightening** of a read whose cost is the monitored server's, argued from write latency measured on the store. Giving it a target-side constant of its own would have invented a one-member regime while the number's actual home is the connection-string builder. **No production code changes here.** **What the pin gained instead is the store-versus-target assertion `.Storage` and `.Viewer` never needed** - those projects only ever talk to the store, and `.Service` holds both kinds of command in the same files, where a target command stamped with `ServiceCommandDeadlines.CollectionSweepSeconds` would look deliberate while bounding the wrong hop. `EverySweepBodyCommand_IsBuiltAgainstTheStore_NotAMonitoredTarget` checks the RECEIVER of all thirteen sites against a word-boundaried allowlist, and asserts the store COUNT as well so it cannot pass by classifying nothing. The boundary guards the direction that matters: without the lookbehind, `sourcepostgres.CreateCommand(` matches `postgres\.CreateCommand` as a substring and a target command on any variable whose name ends in an allowlisted one would be waved through - a false ACCEPT in the guard written to stop one. The store-connection question is asked per MEMBER rather than per file, which this pin's member scoping makes possible and [#2901]'s file-scoped precedent could not, because `DarlingWorker` genuinely holds both kinds. Both directions are bounded STRUCTURALLY rather than by a character window - behind the construction by its own line, ahead of it by its own statement - because these members embed multi-line verbatim SQL that no character count clears. The new controls run through the identical code path the sweep uses, and two of them are the fixtures that matter: the real shape of the target read left out, so enlisting that method fails here rather than silently acquiring a store bound; and a construction quoted in PROSE, which is why every scan in this pin reads stripped source rather than raw.
 - **`memory_pressure_events.sample_time` was written in the monitored server's LOCAL wall clock while both of its readers assume naive UTC, so the memory-pressure series sat one UTC offset away from every other lane** ([#2932]) - `MemoryPressureEventsCollector` derives a stored `sample_time` from `SYSDATETIME()`, and nothing on either read path corrects for it: Darling's `ViewerDataService.Memory` feeds the column straight to `ViewerTimeHelper.ForDisplay`, which is documented as taking naive-UTC input, and Lite's `LocalDataService.Memory` windows it with the UTC-based `GetTimeRange` and then adds `UtcOffsetMinutes` again when plotting. Both consumers want UTC, so the collector is the side that is wrong. Measured on the us-east-2 production store while investigating [#2932]'s read cost: `cpu_utilization_stats.sample_time` sits **exactly 4 h** behind the `collection_time` written by the same collector run on **42 of 42** servers, with the collector reporting SUCCESS **12,526 times in six hours and zero failures** - the same ring-buffer arithmetic off the same `SYSDATETIME()` base, so the memory table carries the same offset. **The CPU collector is deliberately NOT changed, and that is the interesting half.** Its server-local `sample_time` is an intentional, documented convention with a shipped read-side correction: `ViewerDataService.Cpu`'s doc comment states *"Unlike every other stored column, sample_time is the MONITORED SERVER'S LOCAL wall clock (SYSDATETIME() on the server, minus each ring-buffer sample's age), NOT naive UTC"*, and [#1262] de-skews it in SQL per batch (`round(MAX(sample_time) OVER (PARTITION BY server_id, collection_time) - collection_time)` to 15 minutes) while Lite windows it through a purpose-named `GetTimeRangeServerLocal` and shifts it by `ServerTimeHelper.UtcOffsetMinutes`. Converting that column to UTC would contradict [#1262] and break Lite's CPU chart on both the query window and the plotted x-position; Darling would survive only because the per-batch de-skew self-calibrates to zero. So the store does **not** have a single timestamp frame, and `CollectorTimestampFrameTests` now pins the real invariant per column - CPU stays local because [#1262]'s correction depends on it, memory becomes UTC because its readers require it - rather than the frame-wide rule the first cut of this change assumed. Proven red both ways: reverting the memory fix fails the UTC pin, and converting the CPU collector to UTC fails the local pin, each with its mutation's anchor count asserted first. **Not corrected retroactively** - existing memory rows keep the offset they were written with, which is EDT or EST depending on when each landed, so a blanket shift would corrupt half of them; `ServerPropertiesCollector.utc_offset_minutes` already records each server's offset, so a per-row backfill is possible and is left as its own decision. **One operational note:** the first post-fix memory sample jumps forward by the local offset, and because the collector's watermark IS `sample_time` and the jump is forward, whatever remains in the retained ring-buffer window is re-read and stored a second time under its corrected timestamp - a one-time burst of duplicate physical samples at different stamps, not a gap. No schema change and no version bump.
 - **The horizon convergence is now pinned to name `config` and nothing else, so it cannot arm a retention policy** - `ConvergeRetentionHorizonSql` runs on every start, and runs BEFORE the coverage gate, so [#1937]'s doc comment rested the whole thing on one sentence: only `config` is named, therefore "this statement cannot arm anything". Nothing tested it. The statement's text had **zero** test references, and the live convergence test cannot reach it: `EnsureRetentionPoliciesAsync` re-asserts the gate's own verdict through `ArmRetentionPolicySql`/`HoldRetentionPolicySql` one statement later, so the only end state a live test can observe is the gate's. Measured on TimescaleDB 2.29.2 / PostgreSQL 17, splicing either `scheduled => false` or `scheduled => true` into the convergence leaves that test green in **both** directions - while running the spliced statement by hand flips a paused policy to armed, which is the transient armed window [#1680] exists to keep closed, over exactly the tier [#1877] is holding paused because a rollup under it has not yet materialized what the raw table still holds. `RetentionHorizonConvergeCannotArmTests` now reads the named arguments off the SHIPPED string - not a transcription, which would pass forever while the statement drifted - and requires them to be exactly `config`, with neither `scheduled` nor `next_start` appearing anywhere in it. **Every negative is positive-controlled through the identical helper**, because a does-not-contain can pass by matching nothing: the arm/hold pair is the shipped counter-example for `scheduled`, and `next_start`, which nothing ships, is controlled by the shipped statement with the argument spliced in - asserted to differ from the original, so a shape drift that breaks the splice fails loudly instead of controlling nothing. The `next_start` half was already covered behaviourally (the live test's unmoved-`next_start` assertion does fail under mutation); the `scheduled` half had nothing at all.
 - **A second migrator no longer dies undiagnosed waiting on the migration advisory lock, and no longer dies at all when it had nothing to apply** ([#2894]) - [#2888] gave the acquire its own budget, `MigrationLockWaitTimeoutSeconds = 5 * MigrationCommandTimeoutSeconds` (1500 s), derived as four floor-setting data-moving rungs at one per-rung bound each plus one of margin. [#2920] pinned that census. What neither closed is the residual this fixes: the budget was spent by an Npgsql `CommandTimeout` on a blocking `pg_advisory_lock`, and **the operator's entire diagnostic was `Exception while reading from stream`** - measured, not inferred, against PostgreSQL 17.11: Npgsql wraps the expiry as `NpgsqlException` over `TimeoutException("Timeout during reading attempt")`, and the one production caller logs `ex.Message` alone. A 25-minute wait on a sibling migrator reached the log as a sentence about a socket, which is precisely the [#2874] misdiagnosis shape. The acquire now retries `pg_try_advisory_lock` on a one-second cadence, says **"still waiting, N s elapsed of 1500"** every 30 s while it does, and fails with a message naming the lock, both schema versions and the `pg_locks`/`pg_stat_activity` query that finds the holder. **Expiry is deliberately not one answer.** The lock is needed to APPLY rungs, not to decide there are none - the applier commits each rung's DDL and its version stamp in one transaction, so a stamp of N proves every rung up to N committed. So on expiry the stamp table is read once: a store already at `StorageVersion.SchemaVersion` had nothing to apply and continues with 0 applied and a warning, while a store BELOW it has rungs that cannot be applied safely without the lock and throws. That split matters because the case that actually strands an instance is not a slow migrator but a dead one - a session advisory lock outlives its client whenever the server never notices the peer is gone, and then the store is perfectly current while every restart died at the budget for want of a lock it had nothing to do with. **The 1500 s is unchanged and so is its derivation**, because what was unsound was the enforcement, not the number; the census pin needed no edit. **What is still not bounded is a rung that overruns its own budget, and that is now recorded as measured rather than assumed:** `MigrationCommandTimeoutSeconds` is client-side, and neither server-side alternative fits - `statement_timeout` is per STATEMENT, not per transaction (three one-second statements all survive a two-second setting), so it would cost V39's two `CREATE INDEX`es one budget EACH and spend the whole margin, and `transaction_timeout` has the right unit but is PostgreSQL 17+ against readers here that gate as low as 13 and ends the session with FATAL. The waiter is therefore made independent of whether it was covered, which is the part that was reachable. `MigrationLockWaitContentionTests` exercises all of it against a live store under real two-session contention.