From 24afb2b5be2830e58a7dd66334b8621a7f2938dc Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sun, 26 Jul 2026 20:26:19 -0400
Subject: [PATCH 1/6] Pin the wiring, and correct a false claim in the operator
 alert

Two findings, one from each reviewer, both about honesty of a different
kind.

The shepherd caught that my new degraded-alert text said the retained copy
"will not age out on its own". It does. SweepRetainedDataDirectories reads
a MISSING counter as 1, so a failed marker write costs exactly one extra
service start -- the same reasoning we both used to establish that
wrapping the write was safe. Telling an operator otherwise sends them to
delete a multi-gigabyte directory for no reason. Same class of false
operator-facing claim this round was about, just erring toward extra work
instead of false comfort. DarlingSelfAlertEvaluator.cs:1191 now says it
ages out one start later, and names the real signal: the countdown cannot
advance while whatever blocked the write is still blocking it.

The reviewer took my own mutation question and aimed it at the other new
pins, which found that the HIGH fix itself was unpinned. Deleting
`swapped = true`, neutralising the catch filter, dropping the cancellation
guard, or deleting the preflight call all left the entire suite GREEN --
the logic tests pass because the logic is right, and the gated E2E is a
happy path that by construction never throws after the swap and never
meets an occupied port. The defect that reached an arming gate had no test
that would notice it coming back.

Closed with source-parsing pins on the HostHeaderGuardTests idiom, which
exists in this repo for exactly this reason (#1648 was also a wiring
omission a logic test could not see): DarlingStoreUpgradeTests.cs:319 pins
that the post-commit catch stays AHEAD of the general one and never
reverts, :343 that the cancellation path keeps its !swapped guard, :358
that the preflight is actually invoked before pg_upgrade. Catch-clause
order is the mutation most likely to happen by accident -- consolidating
error handling looks harmless, the compiler says nothing, and the
store-bricking path returns silently.

All four mutations verified caught, then reverted. Also documented why
TryStopAsync is safe in the post-commit handler (oldStarted is already
false) so nobody re-derives it.

Full suite 3359 passed, 0 failed, 0 warnings; gated upgrade E2E green end
to end.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 Darling/Darling.Tests/Darling.Tests.csproj    |  5 ++
 .../Darling.Tests/DarlingSelfAlertTests.cs    |  7 +-
 .../Darling.Tests/DarlingStoreUpgradeTests.cs | 78 +++++++++++++++++++
 .../DarlingSelfAlertEvaluator.cs              |  8 +-
 .../DarlingStoreUpgrade.cs                    |  3 +
 5 files changed, 99 insertions(+), 2 deletions(-)

diff --git a/Darling/Darling.Tests/Darling.Tests.csproj b/Darling/Darling.Tests/Darling.Tests.csproj
index c7a9252a87..d96fe3bed7 100644
--- a/Darling/Darling.Tests/Darling.Tests.csproj
+++ b/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
diff --git a/Darling/Darling.Tests/DarlingSelfAlertTests.cs b/Darling/Darling.Tests/DarlingSelfAlertTests.cs
index f0505dc353..b9b07b4227 100644
--- a/Darling/Darling.Tests/DarlingSelfAlertTests.cs
+++ b/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]
diff --git a/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs b/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs
index 8f7917a20e..0e6ed7ccad 100644
--- a/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs
+++ b/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs
@@ -290,6 +290,84 @@ 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);
+        }
+
+        Assert.True(general > filtered,
+            "the unfiltered catch now precedes the post-commit one, so the filtered clause is unreachable. " +
+            "C# takes the first matching handler, the compiler does not warn, and the result is a runtime " +
+            "revert over an already-swapped data directory — an unbootable store.");
+
+        /* 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");
+    }
+
+    [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(
diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs
index 62a37e6cab..d8d98bc34c 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs
@@ -1182,7 +1182,13 @@ says otherwise is the failure this whole review round was about. */
                 var degraded = !string.IsNullOrWhiteSpace(report.FailureMessage);
 
                 var rollback = degraded
-                    ? $" BUT post-upgrade bookkeeping did NOT complete: {report.FailureMessage}. The store itself is fine and running on PostgreSQL {report.ToMajor} — this is about cleanup, not data. Check free disk space on the store volume, and remove the pre-upgrade data directory by hand once you are satisfied with the upgraded store, because it will not age out on its own."
+                    /* The retention counter is DESIGNED to tolerate a missing marker — the sweep reads an
+                       absent counter as 1, so the copy simply ages out one service start later than usual.
+                       Saying it never ages out would send an operator to delete a multi-gigabyte directory by
+                       hand for no reason. What IS true, and is the actual signal: the countdown cannot
+                       advance while whatever blocked the write is still blocking it, because the sweep's own
+                       counter write fails the same way. */
+                    ? $" BUT post-upgrade bookkeeping did NOT complete: {report.FailureMessage}. The store itself is fine and running on PostgreSQL {report.ToMajor} — this is about cleanup, not data. The retained copy's countdown simply starts on the next service start, so it ages out one start later than usual; it only stays put if whatever blocked the write is still blocking it. Check free disk space on the store volume, clear the underlying problem, and remove the directory by hand only if it is still there after a couple of starts."
                     : report.WithoutRollbackCopy
                         ? " The upgrade ran in hard-link mode, so there is NO rollback copy of the pre-upgrade store — the only way back is a restore from backup."
                         : " The pre-upgrade data directory is kept as a rollback copy for the next couple of service starts, then deleted automatically.";
diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs
index 1e272a4246..ab9610d50d 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs
@@ -1356,6 +1356,9 @@ only honest outcome is Succeeded — with the bookkeeping failure said out loud.
                which is precisely the class of self-inflicted damage the move-aside discipline exists to
                prevent; this is that same lesson applied past the swap. So: no revert, no deletion of the new
                data directory (it is the live store now), and no "nothing was modified" claim. */
+            /* Safe here despite pointing at the OLD binaries: oldStarted was set false when the old cluster
+               was stopped before initdb, well before the swap, so this is a no-op on every post-commit path.
+               Spelled out so a future reader does not have to re-derive it and conclude it is a bug. */
             await TryStopAsync(context, oldStarted);
 
             var warning =

From b03692ce84bc76cfb1fa7e415766b9c4593db854 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sun, 26 Jul 2026 20:39:30 -0400
Subject: [PATCH 2/6] Add the CHANGELOG entry for the wiring pins and the alert
 correction

Opened as #1739 after the branch commit was stranded by #1718 merging; the
number is verified against the created PR rather than guessed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 CHANGELOG.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8347004a82..3c71730414 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -65,6 +65,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **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; each of the four mutations was verified to turn them red, then reverted. **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 +1731,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

From e34f2ade04d51fa92b5c972e4721c7cb27d9f92f Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sun, 26 Jul 2026 20:49:35 -0400
Subject: [PATCH 3/6] Pin containment, not just order, on the cancellation
 guard

pg18-bundle-builder re-ran the four mutations independently and found a fifth
the pins missed: moving RevertRuntimeForCancel out of the if (!swapped) block
leaves it textually after the guard, so the order-only assertion passed green,
while making the revert unconditional - the store-bricking path the guard exists
to prevent. It compiles. Closed with a containment assertion; verified the
mutation turns the pin red and the other two stay green, then reverted.

Also corrected the post-commit test comment. Reordering the catch clauses does
not compile at all (CS0160), so the compiler is what prevents that mutation -
the comment claimed the compiler does not warn, and argued the pin was load-
bearing for a case it is not. The test still earns its place on the two
assertions that DO catch silent mutations: the filtered clause existing, and
nothing reverting inside it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 CHANGELOG.md                                  |  2 +-
 .../Darling.Tests/DarlingStoreUpgradeTests.cs | 20 ++++++++++++++++---
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3c71730414..296d0bafe4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -65,7 +65,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
-- **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; each of the four mutations was verified to turn them red, then reverted. **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.
+- **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.
diff --git a/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs b/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs
index 0e6ed7ccad..496d6125fb 100644
--- a/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs
+++ b/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs
@@ -329,10 +329,16 @@ public void PostCommitCatch_StaysAheadOfTheGeneralCatch_AndNeverReverts()
             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 is unreachable. " +
-            "C# takes the first matching handler, the compiler does not warn, and the result is a runtime " +
-            "revert over an already-swapped data directory — an unbootable store.");
+            "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];
@@ -352,6 +358,14 @@ the swap is no more entitled to undo a completed upgrade than an exception is. *
         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 the difference is the whole bug. Moving the revert call OUT of the
+           guarded block leaves it textually after `if (!swapped)` — so the assertion above still passes —
+           while making it run unconditionally, which is precisely the store-bricking path: a shutdown after
+           the swap reverts the runtime and leaves PostgreSQL 17 binaries in front of an 18 data directory.
+           That mutation 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 two. */
+        Assert.DoesNotContain("}", source[guard..revert], StringComparison.Ordinal);
     }
 
     [Fact]

From 20dc186e0c01e9bd3d5e4a8edca236d706b88efc Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sun, 26 Jul 2026 20:51:30 -0400
Subject: [PATCH 4/6] Hand pg_upgrade its credential in a way that works off my
 machine

The gated upgrade fixture went red on its FIRST CI execution, which is
what wiring it into nightly was for -- it had never run anywhere but my
box. pg_upgrade reported:

  connection to server at "localhost" (::1), port 55432 failed:
  FATAL:  password authentication failed for user "darling"

It failed at the dry-run step, so the safety worked exactly as designed:
nothing changed, the runtime reverted, the store came back on 17 with its
data directory untouched. The fail-safe is fine. The upgrade was not.

The credential was right; the HANDOFF was wrong. 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. Four chances to differ
between a developer box and a runner, and it took all four.

Now PGPASSWORD in the child's environment. I chose the file originally to
keep the password out of a process environment block, and that reasoning
does not survive contact: an environment block is readable by the same
user and by administrators, which is the identical audience that can
already read the DPAPI credential file the password comes from -- except
the environment never touches disk, where a killed process could strand a
cleartext temp file the finally never ran for. So it is fewer moving
parts AND less exposure, not a trade. Applies to vacuumdb too.

Also closes a real hole the reviewer found in my own guard test:
CancellationPath_DoesNotRevertOnceTheSwapCommitted asserted textual ORDER,
not CONTAINMENT. Moving the revert OUT of its if (!swapped) block leaves
it after the guard textually and the test stayed green, while a post-swap
shutdown would brick the store again -- the same vacuous-pass shape that
test exists to close, one level in. It now asserts no closing brace falls
between guard and revert, verified by applying that exact mutation and
watching it go red.

Full suite 3359 passed, 0 failed, 0 warnings; gated E2E green end to end.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 CHANGELOG.md                                  |  1 +
 .../Darling.Tests/DarlingStoreUpgradeTests.cs | 13 +++++++
 .../DarlingStoreUpgrade.cs                    | 39 +++++++++++--------
 3 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3c71730414..0abdca4311 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -65,6 +65,7 @@ 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; each of the four mutations was verified to turn them red, then reverted. **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.
diff --git a/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs b/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs
index 0e6ed7ccad..ddc59b4788 100644
--- a/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs
+++ b/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs
@@ -352,6 +352,19 @@ the swap is no more entitled to undo a completed upgrade than an exception is. *
         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 the order-only version of this assertion passed with the bug
+           present. Moving the revert OUT of the guarded block leaves it textually after `if (!swapped)`
+           and green:
+
+               if (!swapped) { TryDeleteDirectory(newDataDirectory); }
+               RevertRuntimeForCancel(context);          // unguarded again
+
+           In correct code the span between the guard and the revert contains no closing brace, because the
+           revert is still inside the block. A `}` there means the block closed first. That is the cheap
+           containment check without needing a parser — and it is the same hole this very test was written
+           to close, one level in. */
+        Assert.DoesNotContain("}", source[guard..revert], StringComparison.Ordinal);
     }
 
     [Fact]
diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs
index ab9610d50d..3eeb68c157 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs
@@ -1115,7 +1115,6 @@ internal async Task<StoreUpgradeOutcome> UpgradeDataDirectoryAsync(UpgradeContex
             + "-upgrade-" + context.NewMajor.ToString(CultureInfo.InvariantCulture);
         var parent = Path.GetDirectoryName(Path.TrimEndingDirectorySeparator(Path.GetFullPath(context.DataDirectory)))!;
         var passwordFile = Path.Combine(parent, "pg-upgrade-pwfile.tmp");
-        var passFile = Path.Combine(parent, "pg-upgrade-pgpass.tmp");
 
         var step = "preflight";
         var oldStarted = false;
@@ -1216,8 +1215,7 @@ can be read rather than assumed ---- */
             AssertUpgradePortsFree();
 
             step = "pg_upgrade-check";
-            WritePgPassFile(passFile, context.UserName, context.Password);
-            var environment = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) { ["PGPASSFILE"] = passFile };
+            var environment = BuildLibpqCredentialEnvironment(context.Password);
 
             var checkExit = await DarlingManagedPostgres.RunDetachingToolAsync(
                 Path.Combine(context.NewBinDirectory, "pg_upgrade.exe"),
@@ -1391,7 +1389,6 @@ it was and reverting the runtime restores a working store. The "never modified"
         finally
         {
             TryDeleteFile(passwordFile);
-            TryDeleteFile(passFile);
         }
     }
 
@@ -1599,14 +1596,27 @@ would only add noise. */
     /// <summary>libpq's password file, so pg_upgrade's internal connections authenticate without an
     /// environment variable carrying the superuser password through the process table. Same
     /// hardened-temp-file discipline as initdb's <c>--pwfile</c>, deleted in the caller's finally.</summary>
-    private static void WritePgPassFile(string path, string userName, string password)
-    {
-        /* Backslash-escape the pgpass field separators. The generated password is alphanumeric, so this is
-           belt-and-braces for a hand-restored credential. */
-        var escaped = password.Replace("\\", "\\\\", StringComparison.Ordinal).Replace(":", "\\:", StringComparison.Ordinal);
-        File.WriteAllText(path, $"*:*:*:{userName}:{escaped}\n");
-        DarlingFileSecurity.HardenFile(path, allowInteractiveRead: false);
-    }
+    /// <summary>
+    /// The credential every libpq tool in the upgrade authenticates with, handed over as <c>PGPASSWORD</c>
+    /// in the child's environment.
+    ///
+    /// <para>This was a hardened <c>PGPASSFILE</c> first, on the reasoning that a file keeps the password out
+    /// of a process environment block. That was wrong on both counts. It FAILED on a CI runner —
+    /// <c>password authentication failed for user "darling"</c> from pg_upgrade's own log, while passing on a
+    /// developer box — because the file approach has to get four separate things right on every host: the
+    /// ACL (and pg_upgrade re-execs itself under a RESTRICTED token on Windows, so the reader is not quite
+    /// the writer), the encoding, a path a restricted child can reach, and cleanup. And it is not actually
+    /// safer: a process environment block is readable by the same user and by administrators, which is the
+    /// identical audience that can already read the DPAPI credential file this password comes from — except
+    /// the environment never touches disk, where a killed process could strand a cleartext temp file the
+    /// <c>finally</c> never ran for.</para>
+    ///
+    /// <para>So: one mechanism, no file, nothing persisted, and the same exposure set. libpq reads
+    /// <c>PGPASSWORD</c> ahead of any password file, and every tool the upgrade spawns (pg_upgrade and the
+    /// pg_dump/pg_restore/psql it spawns in turn) inherits it.</para>
+    /// </summary>
+    private static Dictionary<string, string> BuildLibpqCredentialEnvironment(string password)
+        => new(StringComparer.OrdinalIgnoreCase) { ["PGPASSWORD"] = password };
 
     /// <summary>
     /// pg_upgrade's own logs, which it writes under the NEW data directory and removes on success — so
@@ -1839,10 +1849,8 @@ private async Task RunAnalyzeInStagesAsync(
             return;
         }
 
-        var passFile = Path.Combine(Path.GetTempPath(), $"pm-darling-analyze-{Guid.NewGuid():N}.tmp");
         try
         {
-            WritePgPassFile(passFile, userName, password);
 
             var arguments = new StringBuilder();
             arguments.Append("--host 127.0.0.1 --port ").Append(port.ToString(CultureInfo.InvariantCulture));
@@ -1862,7 +1870,7 @@ private async Task RunAnalyzeInStagesAsync(
                 arguments.ToString(),
                 s_analyzeTimeout,
                 cancellationToken,
-                new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) { ["PGPASSFILE"] = passFile });
+                BuildLibpqCredentialEnvironment(password));
 
             if (exitCode == 0)
             {
@@ -1885,7 +1893,6 @@ private async Task RunAnalyzeInStagesAsync(
         }
         finally
         {
-            TryDeleteFile(passFile);
         }
     }
 }

From 191fe8ec5b19b977da120655b46793dc8928b86b Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sun, 26 Jul 2026 20:57:55 -0400
Subject: [PATCH 5/6] Drop the duplicated CHANGELOG entry the merge left behind

The f58eb19a merge kept BOTH versions of the same entry: the original text
and the corrected one that records the compiler-caught mutation and the fifth
mutation the pins missed. Same title, divergent bodies - which is exactly the
case where a keep-both-sides union is wrong, because the two sides are not two
changes, they are one change edited.

Kept the corrected version, dropped the stale one. The auth-fix entry stays as
its own bullet: it is a genuinely separate user-visible change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 CHANGELOG.md | 1 -
 1 file changed, 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 10707bca2a..74b97d6a7d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -66,7 +66,6 @@ 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; each of the four mutations was verified to turn them red, then reverted. **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.
 - **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.

From 725cbb44d841030cd71a705bfcbce1518dc5fa81 Mon Sep 17 00:00:00 2001
From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com>
Date: Sun, 26 Jul 2026 21:01:29 -0400
Subject: [PATCH 6/6] Remove the doc comment describing the approach that was
 deleted

Review leftovers from the PGPASSFILE removal: the method carried TWO stacked
<summary> elements, the first still describing the password-file approach and
its "deleted in the callers finally" cleanup - false text sitting directly above
the summary explaining why that approach was removed. Same class as the alert
wording defect this PR already fixes, aimed at a maintainer instead of an
operator. Also dropped the empty finally block the removal left behind.

The remaining summary now cites the clause that licenses the deviation rather
than asserting the conclusion: libpq warns against PGPASSWORD "as some operating
systems allow non-root users to see process environment variables via ps", which
is conditioned on an exposure Windows does not have. Records that the value is
set on the CHILD ProcessStartInfo, so the service environment never carries it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 .../DarlingStoreUpgrade.cs                       | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs
index 3eeb68c157..5812d54fd4 100644
--- a/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs
+++ b/Darling/PerformanceMonitor.Darling.Service/DarlingStoreUpgrade.cs
@@ -1593,9 +1593,6 @@ would only add noise. */
         return observedBefore;
     }
 
-    /// <summary>libpq's password file, so pg_upgrade's internal connections authenticate without an
-    /// environment variable carrying the superuser password through the process table. Same
-    /// hardened-temp-file discipline as initdb's <c>--pwfile</c>, deleted in the caller's finally.</summary>
     /// <summary>
     /// The credential every libpq tool in the upgrade authenticates with, handed over as <c>PGPASSWORD</c>
     /// in the child's environment.
@@ -1614,6 +1611,16 @@ would only add noise. */
     /// <para>So: one mechanism, no file, nothing persisted, and the same exposure set. libpq reads
     /// <c>PGPASSWORD</c> ahead of any password file, and every tool the upgrade spawns (pg_upgrade and the
     /// pg_dump/pg_restore/psql it spawns in turn) inherits it.</para>
+    ///
+    /// <para><b>This deliberately departs from libpq's documented advice, and the licence for that is in the
+    /// advice itself.</b> The docs say <c>PGPASSWORD</c> "is not recommended for security reasons, as some
+    /// operating systems allow non-root users to see process environment variables via ps" — a warning
+    /// conditioned on the OS exposing environments to other unprivileged users. Windows does not: reading
+    /// another process's environment block needs PROCESS_VM_READ + PROCESS_QUERY_INFORMATION, which across a
+    /// user boundary requires SeDebugPrivilege, i.e. administrator. The bundled runtime is Windows-only, so
+    /// the premise of that warning never holds here. The value is also set on the CHILD's
+    /// <c>ProcessStartInfo.Environment</c>, never via <c>Environment.SetEnvironmentVariable</c> — so the
+    /// service's own environment never carries it, and it dies with the process tree that needed it.</para>
     /// </summary>
     private static Dictionary<string, string> BuildLibpqCredentialEnvironment(string password)
         => new(StringComparer.OrdinalIgnoreCase) { ["PGPASSWORD"] = password };
@@ -1891,8 +1898,5 @@ private async Task RunAnalyzeInStagesAsync(
         {
             _logger.LogWarning("Post-upgrade analyze could not run ({Message}); autovacuum will build the statistics.", ex.Message);
         }
-        finally
-        {
-        }
     }
 }