From 8dbca640abca02a0cd0b4ceb8c4b9711b6e9b1ae Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 13 Aug 2026 13:47:16 +0200 Subject: [PATCH 1/4] Behavioral coverage for the analyze_now PostgreSQL engine gate (#2230) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three gates from #2213's round-3 fix had source-scanning pins and a live rig run, but nothing drove a PostgreSQL runtime through a gate and asserted the short-circuit. A source scan cannot tell a gate that returns early from one that falls through and happens to write the same text. analyze_now is the tractable one, as #2230 says: its observable is a PRESENCE — a row in analysis_state carrying the engine tombstone. The reconcile and snapshot_now gates observe an ABSENCE (no connection attempted), which still wants a counting seam. The regression guarded is specific and was real: clicking "Generate now" against a PostgreSQL target used to run the full SQL-Server-shaped pass, find nothing, and persist the GENERIC insufficient_data message, overwriting the honest tombstone the scheduled arm had written — so the Recommendations tab regressed from "does not apply, use the PG reads" back to "still collecting" the moment an operator pressed the button. The assertion that matters is therefore not "insufficient_data is true" but that the MESSAGE is the engine one, compared against the shared DarlingWorker.PostgresAnalysisNotApplicable constant. Second test is the discriminator, and without it the first is nearly worthless: a SQL Server target must NOT take the gate. A presence-assertion alone passes just as happily on a gate that fires unconditionally. Reflection for ServerLoopState because it is a PRIVATE nested class inside DarlingWorker — including its List. Deliberately not widened to internal: the repo already reaches private worker state this way (CollectorMemoryKnobTests' gate tests), and changing production accessibility to observe behaviour reflection can already reach is the wrong trade. CommandOutcome is public, so no reflection there. Live-store gated on DARLING_TEST_PG, which CI's "Darling PostgreSQL tests" job sets; the gate's entire effect is a write through _postgres, so there is nothing to observe without one. Cleanup runs on CancellationToken.None per the LiveStoreCleanup convention and deletes only this test's own synthetic server_id. Co-Authored-By: Claude Opus 5 (1M context) --- .../PostgresEngineGateBehaviorTests.cs | 269 ++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs diff --git a/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs b/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs new file mode 100644 index 0000000000..73c0e8fca5 --- /dev/null +++ b/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs @@ -0,0 +1,269 @@ +/* + * 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.Reflection; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging.Abstractions; +using Npgsql; +using PerformanceMonitor.Collectors; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Service; +using Xunit; + +namespace Darling.Tests; + +/// +/// BEHAVIOURAL coverage for the PostgreSQL engine gate on the analyze_now operator door (#2230). +/// +/// What was missing. The three gates added by #2213's round-3 fix were covered by +/// source-scanning pins (TheScheduledAnalysisPassIsGatedByEngine greps the call site) and by a live +/// rig run, but nothing drove a PostgreSQL runtime through a gate and asserted the short-circuit. A source +/// scan cannot tell a gate that returns early from one that falls through and happens to write the same +/// text. +/// +/// Why this gate and not the other two. Its observable is a PRESENCE — a row in +/// analysis_state carrying the engine tombstone — so it can be asserted directly. The reconcile and +/// snapshot_now gates observe an ABSENCE (no connection attempted), which needs a counting seam that does +/// not exist yet; #2230 says as much, and this is the tractable half. +/// +/// The regression it guards is specific and was real. Clicking "Generate now" against a +/// PostgreSQL target used to run the full SQL-Server-shaped pass, find nothing, and persist the GENERIC +/// insufficient_data message — OVERWRITING the honest engine tombstone the scheduled arm had already +/// written. The Recommendations tab regressed from "does not apply, use the PG reads" back to "still +/// collecting" the moment an operator pressed the button. So the assertion that matters is not just +/// "insufficient_data is true", it is that the MESSAGE is the engine one. +/// +/// Live-store gated on DARLING_TEST_PG, which CI's "Darling PostgreSQL tests" job sets — the +/// gate's whole effect is a write through _postgres, so there is nothing to observe without one. +/// +[Collection("live-postgres")] +public sealed class PostgresEngineGateBehaviorTests +{ + /// + /// A server_id far outside the fleet's range, derived the same way the worker derives it, so this test + /// cannot collide with a real server's analysis_state row. + /// + private const string StorageName = "pg-engine-gate-behavior-test-2230"; + + [Fact] + public async Task AnalyzeNow_AgainstAPostgresTarget_WritesTheEngineTombstone_AndDoesNotRunThePass() + { + var connectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrWhiteSpace(connectionString), + "Set DARLING_TEST_PG to a Postgres connection string to run the analyze_now engine-gate test."); + + await using var postgres = NpgsqlDataSource.Create(connectionString!); + var serverId = ServerIdHelper.GetDeterministicHashCode(StorageName); + + /* Fabricated worker, the CollectorMemoryKnobTests.SweepGate idiom: the real ctor wants a host's worth + of dependencies, and the gate under test reads exactly three fields. Reflection because pinning the + BEHAVIOUR beats widening the surface just to observe it. */ + var worker = (DarlingWorker)System.Runtime.CompilerServices.RuntimeHelpers + .GetUninitializedObject(typeof(DarlingWorker)); + SetField(worker, "_serversLock", new object()); + SetField(worker, "_logger", NullLogger.Instance); + SetField(worker, "_postgres", postgres); + + var server = PostgresLoopState(serverId); + var servers = NewLoopStateList(server); + + await CleanupAsync(postgres, serverId); + try + { + var outcome = await InvokeAnalyzeNowAsync(worker, servers, serverId); + + /* 1. The gate returned the success shape, not a failure and not the analysis result. */ + Assert.True(GetOutcomeSuccess(outcome)); + Assert.Equal("analysis not applicable", GetOutcomeStatus(outcome)); + + /* 2. The once-latch is set, so the scheduled tick will not re-write what this just wrote — + the two arms share the tombstone rather than racing to overwrite it. */ + Assert.True(AnalysisStateWritten(server)); + + /* 3. THE REGRESSION GUARD: the persisted message is the ENGINE tombstone, not the generic + insufficient-data text the SQL-Server-shaped pass would have left. */ + var state = await ReadAnalysisStateAsync(postgres, serverId); + var (found, insufficient, message) = (state.Found, state.Insufficient, state.Message); + Assert.True(found, "the gate must PERSIST a row, or the Recommendations tab has nothing to show"); + Assert.True(insufficient); + Assert.Equal(DarlingWorker.PostgresAnalysisNotApplicable, message); + + /* And the specific words that make it honest rather than merely non-empty. */ + Assert.Contains("does not apply to a PostgreSQL target", message, StringComparison.Ordinal); + Assert.Contains("get_pg_blocking", message, StringComparison.Ordinal); + Assert.DoesNotContain("still collecting\"", message, StringComparison.Ordinal); + } + finally + { + await CleanupAsync(postgres, serverId); + } + } + + /// + /// The same door against a SQL Server target must NOT take the gate — otherwise the test above would + /// pass on a gate that fires unconditionally, which is the failure mode a presence-assertion is blind to. + /// Asserted by the outcome status alone: a SQL Server target falls through to the real pass, which + /// on a store with no data for this server_id reports insufficient data. Either way it is NOT + /// "analysis not applicable", and that is the discriminator. + /// + [Fact] + public async Task AnalyzeNow_AgainstASqlServerTarget_DoesNotTakeTheEngineGate() + { + var connectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrWhiteSpace(connectionString), + "Set DARLING_TEST_PG to a Postgres connection string to run the analyze_now engine-gate test."); + + await using var postgres = NpgsqlDataSource.Create(connectionString!); + var serverId = ServerIdHelper.GetDeterministicHashCode(StorageName + "-sqlserver"); + + var worker = (DarlingWorker)System.Runtime.CompilerServices.RuntimeHelpers + .GetUninitializedObject(typeof(DarlingWorker)); + SetField(worker, "_serversLock", new object()); + SetField(worker, "_logger", NullLogger.Instance); + SetField(worker, "_postgres", postgres); + + var server = SqlServerLoopState(serverId); + var servers = NewLoopStateList(server); + + await CleanupAsync(postgres, serverId); + try + { + /* The SQL Server path runs the real analysis pass, which needs collaborators the fabricated + worker does not have — so the assertion is that it did NOT short-circuit as the PG arm, which + is observable either as a different status or as a throw from the pass itself. Both prove the + gate is engine-conditional; only "analysis not applicable" would disprove it. */ + string? status = null; + try + { + status = GetOutcomeStatus(await InvokeAnalyzeNowAsync(worker, servers, serverId)); + } + catch (Exception ex) when (ex is not Xunit.Sdk.XunitException) + { + /* Fell through into the pass and hit a missing collaborator — which is itself the proof. */ + Assert.NotNull(ex); + } + + Assert.NotEqual("analysis not applicable", status); + Assert.False(AnalysisStateWritten(server), + "the PostgreSQL once-latch must not be set for a SQL Server target"); + } + finally + { + await CleanupAsync(postgres, serverId); + } + } + + private static void SetField(DarlingWorker worker, string name, object value) => + typeof(DarlingWorker) + .GetField(name, BindingFlags.NonPublic | BindingFlags.Instance)! + .SetValue(worker, value); + + private static async Task InvokeAnalyzeNowAsync( + DarlingWorker worker, object servers, int serverId) + { + var method = typeof(DarlingWorker).GetMethod( + "RunAnalyzeNowAsync", BindingFlags.NonPublic | BindingFlags.Instance)!; + + /* planFetcher / notificationService / config are only touched on the SQL Server path, so the gate + can be driven with nulls — which is itself part of what "short-circuits" means here. */ + var task = (Task)method.Invoke(worker, new object?[] + { + servers, null, null, null, serverId, CancellationToken.None, + })!; + await task; + return task.GetType().GetProperty("Result")!.GetValue(task)!; + } + + /* CommandOutcome is public (DarlingCommandExecutor), so no reflection is needed for the result — + only for ServerLoopState, which is a private nested type. */ + private static bool GetOutcomeSuccess(object outcome) => ((CommandOutcome)outcome).Success; + + private static string? GetOutcomeStatus(object? outcome) => (outcome as CommandOutcome)?.ResultStatus; + + /// + /// DarlingWorker.ServerLoopState is a PRIVATE nested class, so the test cannot name the type and + /// builds it reflectively — the same trade CollectorMemoryKnobTests makes for private gate state. + /// Widening it to internal purely for a test would be a production change to observe behaviour + /// that reflection can already reach. + /// + private static readonly Type LoopStateType = typeof(DarlingWorker) + .GetNestedType("ServerLoopState", BindingFlags.NonPublic)!; + + private static object NewLoopState(MonitoredServer config, ServerRuntime runtime) + { + var state = Activator.CreateInstance(LoopStateType)!; + LoopStateType.GetProperty("Config")!.SetValue(state, config); + LoopStateType.GetProperty("Runtime")!.SetValue(state, runtime); + return state; + } + + private static bool AnalysisStateWritten(object loopState) => + (bool)LoopStateType.GetProperty("PostgresAnalysisStateWritten")!.GetValue(loopState)!; + + /// The parameter is List<ServerLoopState>, so the list is reflective too. + private static object NewLoopStateList(object single) + { + var list = Activator.CreateInstance(typeof(List<>).MakeGenericType(LoopStateType))!; + list.GetType().GetMethod("Add")!.Invoke(list, new[] { single }); + return list; + } + + private static object PostgresLoopState(int serverId) => NewLoopState( + new MonitoredServer { Name = StorageName, Host = "pg-gate-test.invalid", Engine = "postgres" }, + new ServerRuntime + { + Config = new MonitoredServer { Name = StorageName, Host = "pg-gate-test.invalid" }, + ConnectionString = "Host=pg-gate-test.invalid;Database=postgres;Username=monitor", + Target = new CollectorTargetInfo { Engine = CollectorTargetEngine.PostgreSql }, + StorageName = StorageName, + ServerId = serverId, + }); + + private static object SqlServerLoopState(int serverId) => NewLoopState( + new MonitoredServer { Name = StorageName + "-sqlserver", Host = "sql-gate-test.invalid" }, + new ServerRuntime + { + Config = new MonitoredServer { Name = StorageName + "-sqlserver", Host = "sql-gate-test.invalid" }, + ConnectionString = "Server=sql-gate-test.invalid;Integrated Security=true", + Target = new CollectorTargetInfo { Engine = CollectorTargetEngine.SqlServer }, + StorageName = StorageName + "-sqlserver", + ServerId = serverId, + }); + + private static async Task<(bool Found, bool Insufficient, string Message)> ReadAnalysisStateAsync( + NpgsqlDataSource postgres, int serverId) + { + await using var command = postgres.CreateCommand( + "SELECT insufficient_data, message FROM analysis_state WHERE server_id = $1 " + + "ORDER BY analysis_time DESC LIMIT 1"); + command.Parameters.AddWithValue(serverId); + await using var reader = await command.ExecuteReaderAsync(TestContext.Current.CancellationToken); + if (!await reader.ReadAsync(TestContext.Current.CancellationToken)) + { + return (false, false, string.Empty); + } + + return (true, + !reader.IsDBNull(0) && reader.GetBoolean(0), + reader.IsDBNull(1) ? string.Empty : reader.GetString(1)); + } + + /// + /// Deletes only this test's own synthetic server_id. Runs on CancellationToken.None deliberately, so a + /// cancelled run still leaves the shared live store clean — the LiveStoreCleanup convention. + /// + private static async Task CleanupAsync(NpgsqlDataSource postgres, int serverId) + { + await using var command = postgres.CreateCommand("DELETE FROM analysis_state WHERE server_id = $1"); + command.Parameters.AddWithValue(serverId); + await command.ExecuteNonQueryAsync(CancellationToken.None); + } +} From 5618605c504f16aded2c387f0170cf24bd0fc31e Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 13 Aug 2026 14:24:54 +0200 Subject: [PATCH 2/4] Fix the test's identity derivation: StorageName comes from Host, not Name CI caught it, and the trap is worth recording. MonitoredServer.StorageName is BuildStorageName(Host, Database, ReadOnlyIntent) -- NOT Name -- and RunAnalyzeNowAsync finds a server by hashing that. I hashed a constant that was neither, so the lookup missed and the gate returned 'server not monitored' with Success=false, which is the arm BEFORE the one under test. The serverId is now derived through the same ServerIdHelper.BuildStorageName call the worker uses, so the test cannot drift from the lookup, and each case gets its own unique host. Also reordered the two assertions: status first, because 'server not monitored' names the problem where a bare Assert.True on Success reports only Expected/Actual booleans. That is exactly how much time the original ordering cost. Co-Authored-By: Claude Opus 5 (1M context) --- .../PostgresEngineGateBehaviorTests.cs | 39 ++++++++++++------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs b/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs index 73c0e8fca5..7ba2fafd10 100644 --- a/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs +++ b/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs @@ -48,10 +48,19 @@ namespace Darling.Tests; public sealed class PostgresEngineGateBehaviorTests { /// - /// A server_id far outside the fleet's range, derived the same way the worker derives it, so this test - /// cannot collide with a real server's analysis_state row. + /// The HOST is what identity derives from, which is the trap this test tripped over first: + /// MonitoredServer.StorageName is BuildStorageName(Host, Database, ReadOnlyIntent) — NOT + /// Name — and RunAnalyzeNowAsync finds a server by hashing that. A server_id hashed from + /// anything else simply is not found, and the gate then returns "server not monitored" rather than the + /// arm under test. Unique hosts so neither case can collide with a real server's analysis_state row. /// - private const string StorageName = "pg-engine-gate-behavior-test-2230"; + private const string PgHost = "pg-engine-gate-behavior-2230.invalid"; + + private const string SqlHost = "sql-engine-gate-behavior-2230.invalid"; + + /// Derived through the SAME helper the worker uses, so the test cannot drift from the lookup. + private static int ServerIdFor(string host) => + ServerIdHelper.GetDeterministicHashCode(ServerIdHelper.BuildStorageName(host, null, false)); [Fact] public async Task AnalyzeNow_AgainstAPostgresTarget_WritesTheEngineTombstone_AndDoesNotRunThePass() @@ -61,7 +70,7 @@ public async Task AnalyzeNow_AgainstAPostgresTarget_WritesTheEngineTombstone_And "Set DARLING_TEST_PG to a Postgres connection string to run the analyze_now engine-gate test."); await using var postgres = NpgsqlDataSource.Create(connectionString!); - var serverId = ServerIdHelper.GetDeterministicHashCode(StorageName); + var serverId = ServerIdFor(PgHost); /* Fabricated worker, the CollectorMemoryKnobTests.SweepGate idiom: the real ctor wants a host's worth of dependencies, and the gate under test reads exactly three fields. Reflection because pinning the @@ -81,8 +90,10 @@ BEHAVIOUR beats widening the surface just to observe it. */ var outcome = await InvokeAnalyzeNowAsync(worker, servers, serverId); /* 1. The gate returned the success shape, not a failure and not the analysis result. */ - Assert.True(GetOutcomeSuccess(outcome)); + /* Assert the STATUS first: if the lookup missed, the status is "server not monitored" and says + so, where a bare Assert.True on Success only reports Expected/Actual booleans. */ Assert.Equal("analysis not applicable", GetOutcomeStatus(outcome)); + Assert.True(GetOutcomeSuccess(outcome)); /* 2. The once-latch is set, so the scheduled tick will not re-write what this just wrote — the two arms share the tombstone rather than racing to overwrite it. */ @@ -122,7 +133,7 @@ public async Task AnalyzeNow_AgainstASqlServerTarget_DoesNotTakeTheEngineGate() "Set DARLING_TEST_PG to a Postgres connection string to run the analyze_now engine-gate test."); await using var postgres = NpgsqlDataSource.Create(connectionString!); - var serverId = ServerIdHelper.GetDeterministicHashCode(StorageName + "-sqlserver"); + var serverId = ServerIdFor(SqlHost); var worker = (DarlingWorker)System.Runtime.CompilerServices.RuntimeHelpers .GetUninitializedObject(typeof(DarlingWorker)); @@ -217,24 +228,24 @@ private static object NewLoopStateList(object single) } private static object PostgresLoopState(int serverId) => NewLoopState( - new MonitoredServer { Name = StorageName, Host = "pg-gate-test.invalid", Engine = "postgres" }, + new MonitoredServer { Name = "pg-gate", Host = PgHost, Engine = "postgres" }, new ServerRuntime { - Config = new MonitoredServer { Name = StorageName, Host = "pg-gate-test.invalid" }, - ConnectionString = "Host=pg-gate-test.invalid;Database=postgres;Username=monitor", + Config = new MonitoredServer { Name = "pg-gate", Host = PgHost, Engine = "postgres" }, + ConnectionString = $"Host={PgHost};Database=postgres;Username=monitor", Target = new CollectorTargetInfo { Engine = CollectorTargetEngine.PostgreSql }, - StorageName = StorageName, + StorageName = PgHost, ServerId = serverId, }); private static object SqlServerLoopState(int serverId) => NewLoopState( - new MonitoredServer { Name = StorageName + "-sqlserver", Host = "sql-gate-test.invalid" }, + new MonitoredServer { Name = "sql-gate", Host = SqlHost }, new ServerRuntime { - Config = new MonitoredServer { Name = StorageName + "-sqlserver", Host = "sql-gate-test.invalid" }, - ConnectionString = "Server=sql-gate-test.invalid;Integrated Security=true", + Config = new MonitoredServer { Name = "sql-gate", Host = SqlHost }, + ConnectionString = $"Server={SqlHost};Integrated Security=true", Target = new CollectorTargetInfo { Engine = CollectorTargetEngine.SqlServer }, - StorageName = StorageName + "-sqlserver", + StorageName = SqlHost, ServerId = serverId, }); From 23d88f50f98acf42a3ccc2bac1c06b245bd43449 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 13 Aug 2026 15:28:56 +0200 Subject: [PATCH 3/4] Invert a bogus assertion: the engine message QUOTES 'still collecting' Third self-inflicted failure on this test, and the cheapest to have avoided. The engine tombstone deliberately contains the phrase in order to contrast with it -- 'This is not "still collecting"' -- so DoesNotContain on those words could never pass. My assertion was wrong; the product was right. Now asserts the disclaimer is PRESENT, which is the property actually worth pinning. Verified by reconstructing the constant's runtime value from its C# literal concatenation (the phrase spans two literals in source, so grepping the source for it finds nothing while the runtime string contains it). Co-Authored-By: Claude Opus 5 (1M context) --- Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs b/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs index 7ba2fafd10..3ab21d30fe 100644 --- a/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs +++ b/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs @@ -110,7 +110,11 @@ the two arms share the tombstone rather than racing to overwrite it. */ /* And the specific words that make it honest rather than merely non-empty. */ Assert.Contains("does not apply to a PostgreSQL target", message, StringComparison.Ordinal); Assert.Contains("get_pg_blocking", message, StringComparison.Ordinal); - Assert.DoesNotContain("still collecting\"", message, StringComparison.Ordinal); + /* And it DISCLAIMS the still-collecting reading rather than avoiding the words: the message + quotes the phrase in order to contrast with it ("This is not \"still collecting\""), so a + DoesNotContain on those words can never pass and asserting it was my error, not the + product's. The property worth pinning is that the disclaimer is present. */ + Assert.Contains("This is not \"still collecting\"", message, StringComparison.Ordinal); } finally { From 06df8970aab820464213a1bbfe202ba7e3fd7d95 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 13 Aug 2026 16:19:00 +0200 Subject: [PATCH 4/4] Route the test's teardown through LiveStoreCleanup (#1902 ratchet) CI caught a ratchet I did not know existed: LiveCleanupConversionRatchetTests .NoLiveTestCleansUpOnItsOwnBodysConnection. My finally tore down on the BODY's data source, which is exactly what #1902 closed -- a throw from a finally REPLACES the exception already in flight, and since it is the body's failure that closes the connection, the teardown fails because of the thing it then hides. Two things the ratchet is deliberately strict about, both of which I hit: 1. Opening a fresh connection by hand is NOT accepted. Only LiveStoreCleanup.RunAsync (its own connection) or RunOwnedAsync. Half the fix still throws from the finally. 2. The literal LiveStoreCleanup must appear IN the finally block. My first attempt hid it behind a CleanupAsync helper, which still scanned as an offender -- correctly, as a helper can stop being compliant without the finally changing. Also moved bodySucceeded to be the last statement of each try, per the convention, so a failing body skips the teardown's own error path. Reproduced the ratchet's scan locally this time (2 offenders -> 0) rather than letting CI arbitrate a third round on this file. Co-Authored-By: Claude Opus 5 (1M context) --- .../PostgresEngineGateBehaviorTests.cs | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs b/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs index 3ab21d30fe..05b4a8421c 100644 --- a/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs +++ b/Darling/Darling.Tests/PostgresEngineGateBehaviorTests.cs @@ -84,7 +84,7 @@ BEHAVIOUR beats widening the surface just to observe it. */ var server = PostgresLoopState(serverId); var servers = NewLoopStateList(server); - await CleanupAsync(postgres, serverId); + var bodySucceeded = false; try { var outcome = await InvokeAnalyzeNowAsync(worker, servers, serverId); @@ -115,10 +115,13 @@ quotes the phrase in order to contrast with it ("This is not \"still collecting\ DoesNotContain on those words can never pass and asserting it was my error, not the product's. The property worth pinning is that the disclaimer is present. */ Assert.Contains("This is not \"still collecting\"", message, StringComparison.Ordinal); + + bodySucceeded = true; } finally { - await CleanupAsync(postgres, serverId); + await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, (cleanup, cleanupCt) => + DeleteAnalysisStateAsync(cleanup, cleanupCt, serverId)); } } @@ -148,7 +151,7 @@ public async Task AnalyzeNow_AgainstASqlServerTarget_DoesNotTakeTheEngineGate() var server = SqlServerLoopState(serverId); var servers = NewLoopStateList(server); - await CleanupAsync(postgres, serverId); + var bodySucceeded = false; try { /* The SQL Server path runs the real analysis pass, which needs collaborators the fabricated @@ -169,10 +172,13 @@ public async Task AnalyzeNow_AgainstASqlServerTarget_DoesNotTakeTheEngineGate() Assert.NotEqual("analysis not applicable", status); Assert.False(AnalysisStateWritten(server), "the PostgreSQL once-latch must not be set for a SQL Server target"); + + bodySucceeded = true; } finally { - await CleanupAsync(postgres, serverId); + await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, (cleanup, cleanupCt) => + DeleteAnalysisStateAsync(cleanup, cleanupCt, serverId)); } } @@ -272,13 +278,19 @@ private static object SqlServerLoopState(int serverId) => NewLoopState( } /// - /// Deletes only this test's own synthetic server_id. Runs on CancellationToken.None deliberately, so a - /// cancelled run still leaves the shared live store clean — the LiveStoreCleanup convention. + /// Deletes only this test's own synthetic server_id, through LiveStoreCleanup so the teardown runs + /// on its OWN connection rather than the body's (#1902). A finally that tears down on the body's + /// connection throws out of the finally and REPLACES the body's exception with the teardown's — and it is + /// the body's failure that closed the connection in the first place, so the teardown fails because of the + /// thing it then hides. Opening a fresh connection by hand is explicitly not accepted either: it is half + /// the fix and still throws from the finally. /// - private static async Task CleanupAsync(NpgsqlDataSource postgres, int serverId) + private static async Task DeleteAnalysisStateAsync( + NpgsqlConnection cleanup, CancellationToken cleanupCt, int serverId) { - await using var command = postgres.CreateCommand("DELETE FROM analysis_state WHERE server_id = $1"); + await using var command = new NpgsqlCommand( + "DELETE FROM analysis_state WHERE server_id = $1", cleanup); command.Parameters.AddWithValue(serverId); - await command.ExecuteNonQueryAsync(CancellationToken.None); + await command.ExecuteNonQueryAsync(cleanupCt); } }