Serialize the live classes that share the store, document the ones that do not (#1776)

CHANGELOG.md

@@ -86,6 +86,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - **The directories cluttering a field instance's install folder were invisible to the report built to find them** ([#1785], extends [#1770]) - [#1775]'s report verified SILENT on the production instance: it scans siblings of the DATA directory under `%ProgramData%`, and the seven directories are under the INSTALL directory - a different parent entirely. They would have stayed invisible even in the right place, because that report identifies a store copy structurally by its `PG_VERSION` file and the arithmetic rules clusters out (a ~286 GB data directory on a ~500 GB volume with ~175 GB free - seven copies do not fit), so they are small hand-made snapshots with no store semantics at all. A second report now covers the install directory's top level: any directory the product's layout does not account for is named with its size once per start, and **never deleted** - an absolutism that matters more here than for store copies, because this classifies by ELIMINATION against a known layout rather than by any positive test, so it will sometimes be wrong by construction and reporting is the only verdict that stays harmless when it is. It carries no store semantics either: it can only say the product did not put a directory there, so that is all it says. The known layout is enumerated from the shipped artifacts and measured (`viewer` from the packaging step, `runtimes` and the culture directories from an actual publish, `wwwroot` from the csproj, `pg-runtime` and `pg-runtime-prev` from the runtime paths); locale directories are matched STRUCTURALLY - nothing but `*.resources.dll` - rather than by culture name, because that set is decided by which cultures our dependencies localize into and a hardcoded list would have the product reporting its own shipped directory as foreign. Its own 5-second whole-report budget rather than one shared with the store report, so one report's cost can never degrade another report's content; an exhausted budget degrades a directory's SIZE to "at least" and never drops the directory itself.
+- **Four test classes shared the live Postgres store without serializing against it, and three more were blamed for it by a substring match** ([#1783], closes [#1776]) - classes that read the shared `DARLING_TEST_PG` database without `[Collection("live-postgres")]` ran in parallel with the sixty-odd that have it, and with each other. Three consecutive full-suite runs against one long-lived database failed on a DIFFERENT unrelated class each time, which is the expensive shape of flake: it looks exactly like the change under test broke something it never touched. **CI cannot see it** - it creates a throwaway cluster per run - so the whole cost lands on local development and surfaces as someone else's problem in someone else's pull request, which is why it survived. The classification was verified against the source rather than taken as given, and it moved in both directions. **The most aggressive offender was not on the list**: `DarlingComposeTests` connects to the shared database and runs `PgMigrations.MigrateAsync` - DDL - against it. It does NOT simply take the attribute, because the class holds 127 tests of which 126 are pure and serializing all of them to protect one is the slowdown the issue itself warns against; the live test is split into its own class instead, the shape more than forty files here already use, which also makes the parent's existing claim that "the live round-trip is exercised elsewhere" true rather than aspirational. **Three of the six named were substring artifacts**: `DarlingManagedPostgresTests`, `DarlingStoreUpgradeTests` and `DarlingPgRuntimeVersionPinTests` never read `DARLING_TEST_PG` at all - they read `DARLING_TEST_PGRUNTIME` (and `_OLD` / `_NEWZIP`), different variables that merely share the prefix - so "do not serialize them" was the right answer reached for the wrong reason, and each now records which reason applies so the next sweep does not re-derive the false positive. Two further classes were missed and are genuinely exempt: they reach the variable only to CREATE and DROP their own database through `ScratchPostgres`. **The "transaction-rolled-back" comments those classes carried read like an exemption and are not one** - rolling back protects the DATA, not the CONCURRENCY, since an uncommitted write still holds its row locks and still fires the `config_version` bump trigger; that is now recorded on each, because the comment otherwise invites exactly the wrong conclusion. A new meta-test requires every class reading the shared store to either carry the attribute or record an `#1776 own-store` exemption - a DECISION, not the attribute, since a class with its own database should not serialize - and it matches the QUOTED literal rather than a substring, which is what keeps the three runtime classes and bare prose mentions out; matching the way the original sweep did would leave it permanently red on three innocent classes. It fails rather than skips when it cannot find the source tree, following the doc-comment pin, because a guard that silently skips is a guard that silently stops guarding. Test infrastructure only; no production code changes
 - **One stuck rollback copy froze the ageing-out of every other one** ([#1775], closes [#1770]) - reported from a production field instance carrying seven multi-GB store copies on a volume with roughly 175 GB free against a 286 GB data directory. The retained-copy sweep's failure handling sat OUTSIDE its loop, so a single copy that could not be measured or deleted - a file a not-yet-exited postmaster or an antivirus scan still holds, an ACL the service account lost - abandoned the sweep for every other copy too, and kept abandoning it for as long as the condition lasted. Their retention counters stopped advancing with it, so nothing aged out at all. Each copy now takes its own turn inside its own handler: a failure costs that one directory, names it in the log, and is retried next start. **The seven directories in the report were not ours** - `_rollback_manual_` appears nowhere in the product, and `DarlingStoreUpgrade.cs`, which owns the whole retain-two-starts contract, was created by [#1718]'s work days after they were made. They were made by hand, so they are now REPORTED rather than deleted: any sibling of the data directory holding a `PG_VERSION` file that this service did not create is named every start with its size and a running total, because a copy someone made by hand is a decision to reverse deliberately, not something a service should silently undo - but tens of gigabytes going unmentioned is how a volume reaches that state unnoticed. Identification is structural (`PG_VERSION`), never a name pattern, and the delete stays scoped to the names the upgrade itself produces, re-checked against the prefix because `Directory.GetDirectories` also matches Windows 8.3 short names. A half-built `<datadir>-upgrade-NN` cluster left by an interrupted upgrade gets its own line rather than being blamed on someone else - it is one of ours, deleted only best-effort by the upgrade's failure path - and it is still not removed automatically, because the commit point is two directory moves and a process that died between them leaves the UPGRADED cluster under that name.
 - **The restart delta seed read the whole store to find rows it was about to throw away** ([#1774], closes [#1772]) - reported from a production field instance, where the collector runner logged *"Failed to seed delta calculator from Postgres store... Exception while reading from stream"* on every start, including the one right after an upgrade. The four baseline seed queries carried no time bound on either half, so the inner `MAX(collection_time)` aggregated every chunk of the hypertable and the outer row-value probe scanned them all again - on a 276 GB store, on the default 30-second command timeout. The seed catches its own failures, so what the field actually lost was invisible: restart continuity, silently degraded to first-cycle-zero deltas on every service start. **The bound costs nothing that was being used.** All 36 delta call sites pass `maxGapSeconds: 300`, and the gap policy discards any baseline older than that and returns 0 - the same value an unseeded key returns - so every row found outside a five-minute window was work whose result was already being thrown away. The read is now bounded to a 15-minute window on **both** halves of each query (either one left open reads the whole table), with the cutoff carried as a reused `$1` that both Npgsql and DuckDB resolve to a single parameter, keeping the two apps' queries byte-identical. No timeout was raised. Lite carries the same bound. `memory_grant_stats` also gains `collection_time` in its SELECT list: it was the one family seeding a null timestamp, which disarms the gap policy for two monotonic counters where a stale baseline reads as a fabricated spike. Pinned by mutation on both sides, including against a live PostgreSQL 18.4 + TimescaleDB store, where dropping just the outer bound puts a two-day-old chunk back in the plan.
 - **The service tried to open its own firewall on every start, could never succeed, and a fresh networked install got no rule at all** ([#1773], closes [#1771]) - a production field instance logged both halves of the firewall self-heal failing for BOTH endpoints on EVERY service start: `Remove-NetFirewallRule : Access is denied` and `New-NetFirewallRule : Access is denied`, for 5152 (MCP) and 5153 (web). It was harmless *there* only because the rules already existed from an earlier manual setup. The real defect is what happens without that accident: on a genuinely fresh networked install nothing ever creates the rule, so remote MCP and dashboard clients are firewalled off with only a warning in a log file to say why - and **networked is the normal deployment mode for this product, not the exception**. The cause is structural rather than a bug in the reconcile: the service runs as the `NT SERVICE\PerformanceMonitor Darling` virtual account, which cannot modify Windows Firewall **by design**, and granting it that privilege is the wrong fix - a monitoring service should not hold it.
@@ -1802,6 +1803,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#1762]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1762
 [#1757]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1757
 [#1759]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1759
+[#1776]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1776
 [#1767]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1767
 [#1768]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1768
 [#1770]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1770
@@ -1813,3 +1815,4 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#1785]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1785
 [#1777]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/1777
 [#1780]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1780
+[#1783]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/1783

Darling/Darling.Tests/DarlingAlertTuningKnobsTests.cs

@@ -24,6 +24,10 @@ namespace Darling.Tests;
 /// the V18 delivery-mode pattern. The live test (gated on DARLING_TEST_PG) round-trips the new columns through a
 /// real seed/read against an ISOLATED scratch store.
 /// </summary>
+/* #1776 own-store: deliberately NOT [Collection("live-postgres")]. It reaches DARLING_TEST_PG only to CREATE and
+   DROP its own database through ScratchPostgres, then works entirely inside it — it never touches the shared
+   database's tables, so it cannot race the live collection and serializing it would be pure slowdown. Leave it
+   out; this comment is here so the next sweep does not "fix" it. */
 public sealed class DarlingAlertTuningKnobsTests
 {
     /* ---------------- pure: the long-running-query read shape through the settings seam ---------------- */

Darling/Darling.Tests/DarlingComposeTests.cs

@@ -1897,7 +1897,19 @@ public async Task RunComposedPanel_RejectsBadAbsoluteWindows(string windowJson,
         Assert.NotNull(outcome.Error);
         Assert.Contains(expectedFragment, outcome.Error!, StringComparison.OrdinalIgnoreCase);
     }
+}
 
+/// <summary>
+/// The ONE live compose round-trip, split out of <see cref="DarlingComposeTests"/> (#1776). It connects to the
+/// shared <c>DARLING_TEST_PG</c> store and runs <c>PgMigrations.MigrateAsync</c> against it, so it must serialize
+/// with the rest of the live collection. Its 126 siblings are pure and must NOT: dragging a 127-test class into
+/// the serialized collection to protect one test would cost the suite far more than the race it prevents, and the
+/// established shape here is exactly this pair (more than forty files already split
+/// <c>...SurfaceAndSqlTests</c> from <c>...LivePostgresTests</c> for the same reason).
+/// </summary>
+[Collection("live-postgres")]
+public sealed class DarlingComposeLivePostgresTests
+{
     /// <summary>
     /// The live #1665 repro, end-to-end through the ONE shared runner: a >3-day window against a plain
     /// PostgreSQL store (the darling-pg CI service container — no TimescaleDB, so no rollups exist). Before

Darling/Darling.Tests/DarlingDeliveryModeTests.cs

@@ -32,6 +32,10 @@ namespace Darling.Tests;
 /// live test (gated on DARLING_TEST_PG) round-trips the two settings columns + the per-server override column
 /// through a real seed/read against an isolated scratch store.
 /// </summary>
+/* #1776 own-store: deliberately NOT [Collection("live-postgres")]. It reaches DARLING_TEST_PG only to CREATE and
+   DROP its own database through ScratchPostgres, then works entirely inside it — it never touches the shared
+   database's tables, so it cannot race the live collection and serializing it would be pure slowdown. Leave it
+   out; this comment is here so the next sweep does not "fix" it. */
 public sealed class DarlingDeliveryModeTests
 {
     /* ---------------- pure: the settings seam ---------------- */

Darling/Darling.Tests/DarlingEndpointToggleCliTests.cs

@@ -27,6 +27,11 @@ namespace Darling.Tests;
 /// live test (gated on DARLING_TEST_PG) proves the enable/disable store-writes flip the flag AND self-bump
 /// <c>config_version</c>, transaction-rolled-back so it never clobbers a shared dev store's singleton config row.
 /// </summary>
+/* #1776: the rollback protects the DATA, not the CONCURRENCY — the uncommitted write still holds its row lock on
+   the singleton config row and still fires the self-bump trigger, so unserialized it raced the 63 classes that do
+   carry this attribute (measured: this class failed in the first of three consecutive full-suite runs against one
+   long-lived database). CI creates a throwaway cluster per run and so never sees it. */
+[Collection("live-postgres")]
 public sealed class DarlingEndpointToggleCliTests
 {
     /* ---------------- pure: the four targeted store-write SQL strings ---------------- */

Darling/Darling.Tests/DarlingManagedPostgresTests.cs

@@ -35,6 +35,11 @@ namespace Darling.Tests;
 /// idempotent second EnsureRunning and an ownership-respecting stop — never touching a real
 /// Postgres and never downloading anything.
 /// </summary>
+/* #1776 own-store: deliberately NOT [Collection("live-postgres")], and NOT for the reason a sweep might assume.
+   This class never reads DARLING_TEST_PG at all — it reads DARLING_TEST_PGRUNTIME, which merely shares that
+   prefix, and it stands up its OWN throwaway cluster from the bundled runtime. A substring search for
+   "DARLING_TEST_PG" matches it anyway (that is how #1776's original sweep came to list it), so this note is here to
+   stop the next one serializing a class that touches no shared store. */
 public sealed class DarlingManagedPostgresTests
 {
     [Fact]

Darling/Darling.Tests/DarlingPgRuntimeVersionPinTests.cs

@@ -31,6 +31,11 @@ namespace Darling.Tests;
 ///      equal what the script pins. This is the assertion that would have failed loudly on the stale
 ///      artifact instead of letting it ship and be discovered in the field.
 /// </summary>
+/* #1776 own-store: deliberately NOT [Collection("live-postgres")], and NOT for the reason a sweep might assume.
+   This class never reads DARLING_TEST_PG at all — it reads DARLING_TEST_PGRUNTIME, which merely shares that
+   prefix, and it stands up its OWN throwaway cluster from the bundled runtime. A substring search for
+   "DARLING_TEST_PG" matches it anyway (that is how #1776's original sweep came to list it), so this note is here to
+   stop the next one serializing a class that touches no shared store. */
 public sealed class DarlingPgRuntimeVersionPinTests
 {
     /// <summary>

Darling/Darling.Tests/DarlingStoreUpgradeTests.cs

@@ -35,6 +35,11 @@ namespace Darling.Tests;
 /// its continuous aggregate. That is the UPGRADED-IN-PLACE fixture #1705 proved CI could not see: a store
 /// that has been through a version change behaves differently from the fresh ones darling-pg creates.</para>
 /// </summary>
+/* #1776 own-store: deliberately NOT [Collection("live-postgres")], and NOT for the reason a sweep might assume.
+   This class never reads DARLING_TEST_PG at all — it reads DARLING_TEST_PGRUNTIME_OLD / DARLING_TEST_PGRUNTIME_NEWZIP, which merely shares that
+   prefix, and it stands up its OWN throwaway cluster from the bundled runtime. A substring search for
+   "DARLING_TEST_PG" matches it anyway (that is how #1776's original sweep came to list it), so this note is here to
+   stop the next one serializing a class that touches no shared store. */
 public sealed class DarlingStoreUpgradeTests
 {
     [Theory]

Darling/Darling.Tests/ExcludedDatabasesTests.cs

@@ -124,6 +124,10 @@ public void CollectedDatabaseNamesSql_ReadsDatabaseNameColumnThatExistsInBothSou
 /// is set): the picker's list is the DISTINCT user databases across <c>v_database_config</c> and
 /// <c>v_database_size_stats</c>, system databases removed, resolved from the registry server name.
 /// </summary>
+/* #1776: seeds and reads registry + database rows on the SHARED store, so it must serialize with the 63 classes
+   that already do — measured failing in the third of three consecutive full-suite runs against one long-lived
+   database. Only the live class takes the attribute; the sibling picker tests above touch no store. */
+[Collection("live-postgres")]
 public sealed class ExcludedDatabasesStoreLiveTests
 {
     private const int ServerId = 990_101;