Pin the store-upgrade wiring, and correct a false claim in the operator alert

CHANGELOG.md

@@ -65,6 +65,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **The in-place store upgrade could not authenticate anywhere but a developer's box, and one of its new guard tests could pass with the bug present** ([#1739]) - the gated upgrade fixture went red on its FIRST CI execution, which is exactly what wiring it into nightly was for: `pg_upgrade` reported `password authentication failed for user "darling"` and the upgrade correctly aborted at the dry-run step, reverted, and left the store on 17 with its data directory untouched. The fail-safe worked; the upgrade did not. Cause was the credential HANDOFF, not the credential: it rode a hardened temporary `PGPASSFILE`, which has to get four separate things right on every host - the ACL (and `pg_upgrade` re-executes itself under a RESTRICTED token on Windows, so the reader is not quite the writer), the encoding, a path that restricted child can reach, and cleanup. It now rides `PGPASSWORD` in the child's environment instead, which is also the SAFER option rather than a trade: a process environment block is readable by the same user and by administrators, the identical audience that can already read the DPAPI credential file the password comes from, except nothing touches disk where a killed process could strand a cleartext temp file its `finally` never ran for. Separately, the cancellation-path guard test asserted textual ORDER rather than CONTAINMENT, so moving the revert OUT of its `if (!swapped)` block left it green while a post-swap shutdown would brick the store again - the same vacuous-pass hole that test was written to close, one level in. It now also asserts no closing brace falls between the guard and the revert, verified by applying that exact mutation and watching it go red.
+- **The store-upgrade commit point is now pinned, and the degraded-upgrade alert stops asking for cleanup the product does itself** ([#1739]) - [#1718]'s review round hardened the failure paths; this pins them and corrects the one claim they got wrong. **The wiring had no test.** Mutation-checking the new pins found that deleting `swapped = true`, reordering `catch (Exception ex) when (swapped)` after the unfiltered catch so the filtered clause becomes unreachable, dropping the `!swapped` guard on the cancellation path, or deleting the `AssertUpgradePortsFree()` call ALL left the suite green - the logic tests pass because the logic is right, and the gated end-to-end test is a happy path that structurally cannot reach a failure branch. So the hardest-won guard of that round, the one that stops a post-swap failure putting PostgreSQL 17 binaries in front of an 18 data directory, could have regressed in silence. Closed with source-parsing pins in the `HostHeaderGuardTests` idiom this repo already uses for wiring invariants. Independent re-running of the mutations sharpened two things worth recording rather than smoothing over: reordering the catch clauses does not merely fail a test, it does not COMPILE (`CS0160`), so the compiler is the real guard there and the test comment claiming otherwise was corrected; and a FIFTH mutation was found that the pins missed - moving `RevertRuntimeForCancel` out of the `if (!swapped)` block, which compiles, is exactly the store-bricking bug, and passed green, because the pin asserted textual ORDER rather than CONTAINMENT. Closed with a containment assertion, and that mutation now turns the pin red. **And the alert was wrong.** Its degraded branch told the operator to delete the pre-upgrade data directory by hand "because it will not age out on its own" - but `RollbackRetentionStarts` is 2 and `SweepRetainedDataDirectories` reads a MISSING `.starts` marker as 1, so it writes the counter, keeps the copy, and deletes it on the following start. A failed marker write costs exactly one extra service start, which is the same reasoning that made wrapping that write safe in the first place. It now says so, with the real conditional: the copy only stays put while whatever blocked the write is still blocking it. Same defect class as the round that produced it - operator-facing text asserting something the code does not do - erring toward unnecessary work rather than false comfort.
 - **Azure SQL Database: `database_size_stats` stopped touching `master`, and error 300 now explains itself** ([#1732]) - the remaining half of #1631, reported by TrudAX on a Dynamics 365 elastic-pool database reached through a DATABASE-level firewall rule. **`database_size_stats`** kept throwing `40615 Cannot open server ... not allowed to access the server` even after [#1634]/[#1642]. The query was never the problem - it was already database-scoped - but the collector declared `RunsPerDatabase` on Azure, and that made the host ENUMERATE databases first, which connects to `master`: the one database that login cannot open. The enumeration bought nothing there, because the connection already points at the database being monitored, the query reads only `sys.database_files` + `FILEPROPERTY(SpaceUsed)` (satisfied by the `public` role), and a contained Azure user cannot see sibling databases anyway - so the databases it went to `master` to discover were never readable. It now runs once on the existing connection and `master` is out of this collector's path entirely, rather than relying on #1634's fallback to recover from an error it never needed to provoke. Pinned so `sys.master_files`, `dm_os_volume_stats` and `dm_db_file_space_usage` cannot creep back in - the last of those looks like the natural Azure choice but carries the very trap described next. **`waiting_tasks`** answers the question TrudAX actually asked ("does it really require master DB access?"): **no, and no amount of `master` access would help.** Per MS Learn, `VIEW DATABASE STATE` covers `sys.dm_os_waiting_tasks` on every Azure SQL Database service objective EXCEPT Basic, S0, S1, and any database in an **elastic pool** - on those, only the server admin, the Entra admin, or a login in `##MS_ServerStateReader##` can read it, and `VIEW SERVER STATE` is not grantable at the server on that platform at all. So the collector is NOT gated off Azure (that would break it for the majority of Azure users, on S2+/vCore, for whom it works); instead the stored error now names the real cause and both remedies, appended to the raw SQL error so it stays searchable. Lite and Darling both, identical wording.
 - **Lite: prove the Agent-status SQL, not just the decision it feeds** ([#1730]) - [#1725]'s tests all run against constructed `AgentStatusRow` values, which is the right shape for pinning what a row MEANS but leaves the query that produces those rows unproven. It is not trivial SQL: `ever_seen_running` is a window aggregate that has to see the whole retained partition while the surrounding query collapses to the newest row per server, and the two ways that goes wrong both return a plausible boolean and both pass every model-level test. If it collapsed to the newest row, a server that ran Agent yesterday and stopped today would read "never ran Agent" and a **genuine outage would render neutral** - the exact failure the fix exists to prevent, arrived at from the other direction. If the partition leaked, one real server's history would make every Agent-less container read as a server that runs Agent, putting the red "Stopped" straight back where [#1725] removed it. Six real-DuckDB round-trips now cover both, plus the newest-row-wins tiebreak, `collection_time` driving staleness, and the fresh case NOT being suppressed. Confirmed non-vacuous by mutation: dropping `PARTITION BY server_id` from the shipped query fails two of them. Also pins that the stale window stays longer than the collector's own cadence, read from `CollectorScheduleDefaults` so it follows a retune - a live trap rather than a hypothetical, since the shared `ServerHealthThresholds.StaleThreshold` is two minutes derived from the FASTEST collector's one-minute cadence, looks like the obvious thing to unify this with, and would render a perfectly healthy Agent as "unknown (stale)" most of the time because `agent_status` collects every five. Tests only; no production code, and the shipped query passes as written.
 - **Credential-file ACL failures now name the owner, and the DPAPI credential files verify the result instead of assuming it** ([#1727]) - hardening #1721. When the service cannot re-ACL a file it protects, the error said "fix the file permissions by hand" and left the operator to work out which permissions, held by whom, and why. It now names the file's OWNER and the ordinary-user group that can read it, and states the part that actually resolves it: `SetAccessControl` needs WRITE_DAC, which comes with ownership or FullControl, so a service account holding only inherited Modify on a file owned by someone else can NEVER succeed - restarting will not clear it, and only granting FullControl or ownership will. That was the exact live condition on a field box, where the same error repeated every start for a day. The bigger half: the post-harden verification that `darling.json` already had - attempt, then CHECK whether ordinary users can still read the bytes, and log Critical if they can - is now applied to the machine-scoped DPAPI **credential** files too (the managed-Postgres owner/admin/viewer/MCP credentials and the least-privilege role credentials), which previously only logged the attempt. For LocalMachine DPAPI, read access to the file IS the secret, so "we tried to harden it" and "the secret is protected" are different claims and only one of them is worth logging.
@@ -1730,6 +1732,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#1734]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1734
 [#1725]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1725
 [#1730]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1730
+[#1739]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1739
 [#1727]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1727
 [#1690]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1690
 [#1693]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1693

Darling/Darling.Tests/Darling.Tests.csproj

@@ -46,6 +46,11 @@
          never sees them). HostHeaderGuardTests parses them to pin that the #1648 DNS-rebinding middleware is
          installed FIRST and in BOTH bind modes: the defect was a WIRING omission, not a logic bug, so the pure
          decision test alone would have passed on the broken build. -->
+    <!-- #1706: the store-upgrade orchestration is parsed as SOURCE to pin two WIRING invariants that no
+         behavioral test can reach — the post-commit catch must stay ahead of the general one, and the port
+         preflight must actually be called. Both defects are invisible to logic tests and to the happy-path
+         E2E, exactly like the #1648 middleware-ordering case these fixtures already exist for. -->
+    <None Include="..\PerformanceMonitor.Darling.Service\DarlingStoreUpgrade.cs" Link="Fixtures\DarlingStoreUpgrade.cs" CopyToOutputDirectory="PreserveNewest" />
     <None Include="..\PerformanceMonitor.Darling.Service\Mcp\DarlingMcpHostService.cs" Link="Fixtures\DarlingMcpHostService.cs" CopyToOutputDirectory="PreserveNewest" />
     <None Include="..\PerformanceMonitor.Darling.Service\Mcp\DarlingWebHostService.cs" Link="Fixtures\DarlingWebHostService.cs" CopyToOutputDirectory="PreserveNewest" />
     <!-- The runtime fetch script, copied beside the test binary so DarlingPgRuntimeVersionPinTests reads

Darling/Darling.Tests/DarlingSelfAlertTests.cs

@@ -953,7 +953,12 @@ await e.EvaluateStoreUpgradeAsync(
         /* And the reassuring sentence must be REPLACED, not merely appended to — the retention marker is
            precisely what failed, so promising it ages out automatically would be actively wrong. */
         Assert.DoesNotContain("then deleted automatically", fired.DetailText, StringComparison.Ordinal);
-        Assert.Contains("remove the pre-upgrade data directory by hand", fired.DetailText, StringComparison.Ordinal);
+        /* The corrected wording: the copy DOES age out, one start later, because the sweep reads a
+           missing counter as 1. Telling an operator it never ages out would send them to delete a
+           multi-gigabyte directory for no reason — the same class of false operator-facing claim this
+           whole round was about, just erring toward extra work instead of false comfort. */
+        Assert.Contains("ages out one start later than usual", fired.DetailText, StringComparison.Ordinal);
+        Assert.DoesNotContain("will not age out on its own", fired.DetailText, StringComparison.Ordinal);
     }
 
     [Fact]

Darling/Darling.Tests/DarlingStoreUpgradeTests.cs

@@ -290,6 +290,104 @@ public void BuildStoreUpgradeReport_CleanSuccessCarriesNoWarning_AndExtensionOnl
         Assert.Null(DarlingWorker.BuildStoreUpgradeReport(DarlingStoreUpgrade.StoreUpgradeOutcome.None));
     }
 
+    /* ==================================================================================
+       WIRING pins, parsed from source.
+
+       Both defects below are invisible to every other kind of test here. The logic tests pass because the
+       logic is correct; the gated E2E passes because it is a HAPPY path that never throws after the swap and
+       never meets an occupied port. Deleting `swapped = true`, reordering the catch clauses, or deleting the
+       preflight call leaves the whole suite green — which is exactly the hole that let the original HIGH
+       reach an arming gate. Same reasoning, and the same idiom, as HostHeaderGuardTests' #1648 middleware
+       ordering pins: a WIRING omission needs a wiring test.
+       ================================================================================== */
+
+    private static string ReadUpgradeSource()
+    {
+        var path = Path.Combine(AppContext.BaseDirectory, "Fixtures", "DarlingStoreUpgrade.cs");
+        Assert.True(File.Exists(path),
+            "DarlingStoreUpgrade.cs was not copied beside the test binary — check the csproj None/Link item.");
+
+        var source = File.ReadAllText(path);
+        /* Guard the guard: if the file were restructured past recognition these assertions could pass
+           vacuously on a mismatched parse, so pin the anchors they key off. */
+        Assert.Contains("swapped = true;", source, StringComparison.Ordinal);
+        Assert.Contains("catch (Exception ex)", source, StringComparison.Ordinal);
+        return source;
+    }
+
+    [Fact]
+    public void PostCommitCatch_StaysAheadOfTheGeneralCatch_AndNeverReverts()
+    {
+        var source = ReadUpgradeSource();
+
+        var filtered = source.IndexOf("catch (Exception ex) when (swapped)", StringComparison.Ordinal);
+        Assert.True(filtered >= 0, "the post-commit catch clause is gone — a failure after the swap would revert the runtime and leave old binaries in front of a new data directory");
+
+        var general = source.IndexOf("catch (Exception ex)\r\n", filtered, StringComparison.Ordinal);
+        if (general < 0)
+        {
+            general = source.IndexOf("catch (Exception ex)\n", filtered, StringComparison.Ordinal);
+        }
+
+        /* Belt-and-braces, and worth being honest about why: reordering these two clauses does NOT compile
+           (CS0160, "a previous catch clause already catches all exceptions of this or of a super type"), so
+           the compiler — not this assertion — is what actually prevents that specific mutation. This test
+           earns its place on the two assertions around this one: that the filtered clause exists at all, and
+           that nothing inside it reverts. Both of those mutations compile silently. */
+        Assert.True(general > filtered,
+            "the unfiltered catch now precedes the post-commit one, so the filtered clause would be " +
+            "unreachable — a runtime revert over an already-swapped data directory, which is an unbootable " +
+            "store. If this ever fails rather than failing to compile, the structure has changed in a way " +
+            "that needs a human to look at it.");
+
+        /* And within the post-commit handler, nothing may revert. */
+        var body = source[filtered..general];
+        Assert.DoesNotContain("RevertRuntime", body, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void CancellationPath_DoesNotRevertOnceTheSwapCommitted()
+    {
+        var source = ReadUpgradeSource();
+        var cancel = source.IndexOf("catch (OperationCanceledException)", StringComparison.Ordinal);
+        Assert.True(cancel >= 0);
+
+        /* The revert in the cancellation handler must sit behind the !swapped guard. A shutdown timed after
+           the swap is no more entitled to undo a completed upgrade than an exception is. */
+        var guard = source.IndexOf("if (!swapped)", cancel, StringComparison.Ordinal);
+        var revert = source.IndexOf("RevertRuntimeForCancel", cancel, StringComparison.Ordinal);
+        Assert.True(guard >= 0 && revert > guard,
+            "the cancellation path reverts the runtime without checking whether the swap already committed");
+
+        /* ORDER IS NOT CONTAINMENT, and that difference is the whole bug. Moving the revert call OUT of
+           the guarded block leaves it textually after `if (!swapped)`, so an order-only assertion still
+           passes while the call now runs unconditionally:
+
+               if (!swapped) { TryDeleteDirectory(newDataDirectory); }
+               RevertRuntimeForCancel(context);          // unguarded again
+
+           That is the store-bricking path — a shutdown after the swap reverts the runtime and leaves
+           PostgreSQL 17 binaries in front of an 18 data directory. It compiles, and it passed this test
+           green until this line existed. Correct code has NO closing brace between the guard and the call;
+           relocating the call introduces one. Cheap containment without needing a parser — and the same
+           vacuous-pass shape this very test was written to close, one level in. */
+        Assert.DoesNotContain("}", source[guard..revert], StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void PortPreflight_IsActuallyInvokedBeforePgUpgradeRuns()
+    {
+        var source = ReadUpgradeSource();
+
+        var call = source.IndexOf("AssertUpgradePortsFree();", StringComparison.Ordinal);
+        Assert.True(call >= 0,
+            "nothing calls AssertUpgradePortsFree — FindOccupiedPorts being correct is worth nothing if the " +
+            "check never runs, and a clean CI runner has free ports so no behavioral test would notice");
+
+        var firstUpgrade = source.IndexOf("pg_upgrade.exe", StringComparison.Ordinal);
+        Assert.True(firstUpgrade > call, "the port preflight must run BEFORE pg_upgrade is invoked");
+    }
+
     [Fact]
     public void RetainedDataDirectory_IsNamedForTheMajorItCameFrom()
         => Assert.Equal(