diff --git a/CHANGELOG.md b/CHANGELOG.md index a9b874559b..bad8df71d0 100644 --- a/CHANGELOG.md +++ b/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 `-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 diff --git a/Darling/Darling.Tests/DarlingAlertTuningKnobsTests.cs b/Darling/Darling.Tests/DarlingAlertTuningKnobsTests.cs index 09116e7010..2491427a8d 100644 --- a/Darling/Darling.Tests/DarlingAlertTuningKnobsTests.cs +++ b/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. /// +/* #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 ---------------- */ diff --git a/Darling/Darling.Tests/DarlingComposeTests.cs b/Darling/Darling.Tests/DarlingComposeTests.cs index 6f1444fa69..e6fd208ea7 100644 --- a/Darling/Darling.Tests/DarlingComposeTests.cs +++ b/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); } +} +/// +/// The ONE live compose round-trip, split out of (#1776). It connects to the +/// shared DARLING_TEST_PG store and runs PgMigrations.MigrateAsync 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 +/// ...SurfaceAndSqlTests from ...LivePostgresTests for the same reason). +/// +[Collection("live-postgres")] +public sealed class DarlingComposeLivePostgresTests +{ /// /// 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 diff --git a/Darling/Darling.Tests/DarlingDeliveryModeTests.cs b/Darling/Darling.Tests/DarlingDeliveryModeTests.cs index 4bdac06ebe..31a5089645 100644 --- a/Darling/Darling.Tests/DarlingDeliveryModeTests.cs +++ b/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. /// +/* #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 ---------------- */ diff --git a/Darling/Darling.Tests/DarlingEndpointToggleCliTests.cs b/Darling/Darling.Tests/DarlingEndpointToggleCliTests.cs index 631726b242..af0f35acc0 100644 --- a/Darling/Darling.Tests/DarlingEndpointToggleCliTests.cs +++ b/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 /// config_version, transaction-rolled-back so it never clobbers a shared dev store's singleton config row. /// +/* #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 ---------------- */ diff --git a/Darling/Darling.Tests/DarlingManagedPostgresTests.cs b/Darling/Darling.Tests/DarlingManagedPostgresTests.cs index d528e32a01..f273a033ac 100644 --- a/Darling/Darling.Tests/DarlingManagedPostgresTests.cs +++ b/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. /// +/* #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] diff --git a/Darling/Darling.Tests/DarlingPgRuntimeVersionPinTests.cs b/Darling/Darling.Tests/DarlingPgRuntimeVersionPinTests.cs index 859f40a307..b7966efbc3 100644 --- a/Darling/Darling.Tests/DarlingPgRuntimeVersionPinTests.cs +++ b/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. /// +/* #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 { /// diff --git a/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs b/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs index 5653adc0c9..f8d8a09916 100644 --- a/Darling/Darling.Tests/DarlingStoreUpgradeTests.cs +++ b/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. /// +/* #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] diff --git a/Darling/Darling.Tests/ExcludedDatabasesTests.cs b/Darling/Darling.Tests/ExcludedDatabasesTests.cs index c2c3cbc0ea..0d571a5a0f 100644 --- a/Darling/Darling.Tests/ExcludedDatabasesTests.cs +++ b/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 v_database_config and /// v_database_size_stats, system databases removed, resolved from the registry server name. /// +/* #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; diff --git a/Darling/Darling.Tests/LivePostgresCollectionHygieneTests.cs b/Darling/Darling.Tests/LivePostgresCollectionHygieneTests.cs new file mode 100644 index 0000000000..9105ead728 --- /dev/null +++ b/Darling/Darling.Tests/LivePostgresCollectionHygieneTests.cs @@ -0,0 +1,205 @@ +/* + * Copyright (c) 2026 Erik Darling, Darling Data LLC + * + * This file is part of the SQL Server Performance Monitor. + * + * Licensed under the MIT License. See LICENSE file in the project root for full license information. + */ + +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.RegularExpressions; +using Xunit; + +namespace Darling.Tests; + +/// +/// Every test class that touches the shared DARLING_TEST_PG store must either serialize against the other +/// live classes or say in writing why it does not (#1776). +/// +/// The failure this prevents is a moving one. Six classes connected to the shared store without +/// [Collection("live-postgres")], so they ran in parallel with the sixty-odd classes that carry 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 catch this, because it creates a throwaway cluster per run, +/// so the cost lands entirely on local development and shows up as someone else's problem in someone else's pull +/// request. That is precisely why it survived so long, and why the guard has to be a test rather than a habit. +/// +/// The exemption is real and must stay available. A class that mints its own database (through +/// ScratchPostgres) or stands up its own cluster does not race the shared store, and serializing it would +/// cost suite time for no safety at all. So this does not demand the attribute — it demands a DECISION, recorded +/// either as the attribute or as an #1776 own-store comment explaining the exemption. +/// +/// Why the match is the quoted literal and not a substring. DARLING_TEST_PGRUNTIME and +/// DARLING_TEST_PGRUNTIME_OLD are DIFFERENT variables that merely share the prefix, naming an assembled +/// runtime directory rather than a store. #1776's original sweep matched on substring and so listed three classes +/// that never read DARLING_TEST_PG at all, reaching the right answer for the wrong reason. Matching the +/// quoted literal keeps those out, and keeps out prose mentions in doc comments (which write the name bare) so a +/// class that only DESCRIBES the variable is not dragged in. +/// +public sealed class LivePostgresCollectionHygieneTests +{ + /// The env-var read as it appears in code — quoted, so DARLING_TEST_PGRUNTIME cannot match. + private const string SharedStoreVariable = "\"DARLING_TEST_PG\""; + + /// + /// The scratch-database helper's factory call. Reaching the store THROUGH the helper counts as reaching the + /// store, because that is what it does: ScratchPostgres.CreateAsync connects to the + /// DARLING_TEST_PG server and issues CREATE DATABASE on it. + /// + /// Without this the guard would only work by ACCIDENT — it would be detecting the env-var read that + /// each consumer happens to duplicate at its own call site, not the store access. A routine DRY refactor + /// moving that read inside the helper would blind the rule for every consumer at once, silently, which is + /// exactly the failure it exists to prevent. Keying on the helper as well means the rule follows REACHING + /// the store rather than the spelling of how you got there, so that refactor stays legal instead of being + /// quietly forbidden. + /// + /// Matched with the trailing .CreateAsync( for the same reason the variable is matched quoted: + /// prose says "through ScratchPostgres" and a test method is named …AgainstScratchPostgres, + /// neither of which is a store access. The helper's own file declares Task<ScratchPostgres> + /// CreateAsync( and new ScratchPostgres(, so it does not match itself either. + /// + private const string ScratchStoreFactory = "ScratchPostgres.CreateAsync("; + + private const string LiveCollectionAttribute = "[Collection(\"live-postgres\")]"; + + /// The recorded-exemption marker. Prose, deliberately: the point is that a human wrote down why. + private const string OwnStoreMarker = "#1776 own-store"; + + /// Top-level class declarations. Nested types are indented and correctly not matched. + private static readonly Regex ClassDeclaration = + new(@"^(?:public|internal)\s+(?:sealed\s+|static\s+|abstract\s+|partial\s+)*class\s+(\w+)", + RegexOptions.Compiled | RegexOptions.Multiline); + + /// + /// How far above a class declaration to look for its attribute or exemption marker. Generous enough for an + /// attribute sitting above a long doc comment, tight enough that it cannot reach into the previous class's body. + /// + private const int HeaderLookbackLines = 25; + + [Fact] + public void EveryClassUsingTheSharedStore_IsSerializedOrDocumentsWhyNot() + { + var directory = FindTestProjectDirectory(); + + /* FAIL rather than skip when the source tree cannot be found — the DocCommentHygieneTests rule: a guard + that silently skips is a guard that silently stops guarding. */ + Assert.True(directory is not null, + "Could not locate the Darling.Tests source directory (walked up from the test binary looking for " + + "PerformanceMonitor.sln). This test scans source, so it cannot run without it — fix the walk-up " + + "rather than skipping, or the rule stops being enforced without anyone noticing."); + + var offenders = new List(); + + /* Recurse, and skip build output. The project is flat today, so TopDirectoryOnly would be equivalent — + but it would also mean the first test file someone puts in a subfolder escapes the rule silently, which + is the failure mode this whole test exists to prevent. Scoping to THIS project is correct rather than + lazy: [Collection] groups classes within an assembly, so a class elsewhere could not join the live + collection even if it wanted to, and the quoted literal appears nowhere else in the repo. */ + foreach (var file in Directory.EnumerateFiles(directory!, "*.cs", SearchOption.AllDirectories)) + { + if (file.Contains(@"\bin\", StringComparison.OrdinalIgnoreCase) + || file.Contains(@"\obj\", StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + var text = File.ReadAllText(file); + if (!TouchesSharedStore(text)) + { + continue; + } + + var declarations = ClassDeclaration.Matches(text); + for (var i = 0; i < declarations.Count; i++) + { + var declaration = declarations[i]; + var bodyEnd = i + 1 < declarations.Count ? declarations[i + 1].Index : text.Length; + var body = text[declaration.Index..bodyEnd]; + + if (!TouchesSharedStore(body)) + { + continue; + } + + var header = HeaderAbove(text, declaration.Index); + if (header.Contains(LiveCollectionAttribute, StringComparison.Ordinal) + || header.Contains(OwnStoreMarker, StringComparison.Ordinal)) + { + continue; + } + + var line = text.AsSpan(0, declaration.Index).Count('\n') + 1; + offenders.Add($"{Path.GetFileName(file)}:{line} {declaration.Groups[1].Value}"); + } + } + + Assert.True(offenders.Count == 0, + "These classes reach the shared DARLING_TEST_PG store — directly, or through ScratchPostgres, which " + + "mints its database on that server — but neither serialize against the other live classes nor " + + "record why they do not:\n\n" + + string.Join("\n", offenders) + + "\n\nPick one, deliberately:\n" + + $" - Add {LiveCollectionAttribute} if the class touches the SHARED database. Rolling the writes " + + "back does not exempt it: an uncommitted write still holds its row locks and still fires triggers.\n" + + $" - Add an \"{OwnStoreMarker}\" comment if the class mints its own database or cluster and so " + + "cannot race the shared one. Serializing those is pure slowdown.\n\n" + + "If the class is mostly pure with one live test, prefer SPLITTING the live test into its own " + + "...LivePostgresTests class (the shape more than forty files here already use) over serializing " + + "every pure test alongside it."); + } + + /// + /// Does this source reach the shared store — directly by reading DARLING_TEST_PG, or through the + /// scratch-database helper, which mints its database ON that server? Either counts. + /// + private static bool TouchesSharedStore(string source) => + source.Contains(SharedStoreVariable, StringComparison.Ordinal) + || source.Contains(ScratchStoreFactory, StringComparison.Ordinal); + + /// + /// The class's own attribute/comment region: the lines immediately above the + /// declaration. Bounded so it cannot reach back into the previous class's body and read ITS attribute. + /// + private static string HeaderAbove(string text, int declarationIndex) + { + var start = declarationIndex; + for (var lines = 0; lines < HeaderLookbackLines && start > 0; lines++) + { + var previous = text.LastIndexOf('\n', start - 1); + if (previous < 0) + { + start = 0; + break; + } + + start = previous; + } + + return text[start..declarationIndex]; + } + + /// + /// Walks up from the test output directory to the repo root (the directory holding + /// PerformanceMonitor.sln) and returns this project's source directory. Same walk-up idiom as + /// DocCommentHygieneTests.FindRepoRoot. + /// + private static string? FindTestProjectDirectory() + { + var directory = new DirectoryInfo(AppContext.BaseDirectory); + for (var i = 0; i < 10 && directory is not null; i++) + { + if (File.Exists(Path.Combine(directory.FullName, "PerformanceMonitor.sln"))) + { + var source = Path.Combine(directory.FullName, "Darling", "Darling.Tests"); + return Directory.Exists(source) ? source : null; + } + + directory = directory.Parent; + } + + return null; + } +} diff --git a/Darling/Darling.Tests/StoreConfigProviderTests.cs b/Darling/Darling.Tests/StoreConfigProviderTests.cs index d3f2e82c1e..c90fab6860 100644 --- a/Darling/Darling.Tests/StoreConfigProviderTests.cs +++ b/Darling/Darling.Tests/StoreConfigProviderTests.cs @@ -26,6 +26,13 @@ namespace Darling.Tests; /// (gated on DARLING_TEST_PG) prove the V17 bump trigger fires and are transaction-rolled-back so they never /// clobber a shared dev store's singleton config rows. /// +/* #1776: rolling the writes back protects the DATA, not the CONCURRENCY — an uncommitted write still holds its + row locks and still fires the config_version bump trigger, so running unserialized against the shared store + raced the 63 classes that do carry this attribute. Measured over three consecutive full-suite runs against one + long-lived database: this class failed in run 1, and the failures landed on a DIFFERENT unserialized class each + run, which is what makes the class of bug so expensive — it looks like the change under test broke something it + never touched. CI never sees it because it creates a throwaway cluster per run. */ +[Collection("live-postgres")] public sealed class StoreConfigProviderTests { /* ---------------- pure: apply (view -> held config, by-reference seam) ---------------- */