A store with no readable log directory keeps its hourly collector-cost flush

Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs

@@ -943,7 +943,7 @@ private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScop
     };
 
     private const int WorkerCountedSites = 10;
-    private const int WorkerExemptSites = 8;
+    private const int WorkerExemptSites = 9;
 
     /// <summary>
     /// Counted sites tree-wide. ONE numeral with several readers rather than the same number written out at
@@ -1013,6 +1013,7 @@ private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScop
         ["could not read the store volume free space"] = "a local filesystem read, not a store read",
         ["could not read the recorded store size"] = "context for the alert text, not the evidence the alert is judged on",
         ["Store self-metrics sweep did not finish"] = "a metrics write sweep; no alert is judged on its result",
+        ["so this hour's size rows are missing"] = "a metrics write sweep (#3923); no alert is judged on its result, and the store-log census and collector-cost flush the same tick still run on the connection this narrow catch deliberately leaves open",
         ["Store log capture failed"] = "a telemetry write sweep (#3021); no alert is judged on its result, and the capture gap it leaves is reported by get_store_log's own denominator",
         ["Store log: re-masking rows captured before this build failed"] = "a maintenance rewrite of stored store-log text (#3915); no alert is judged on it, it is idempotent and resumes next hour, and until it finishes the store-log reader masks every row on the way out",
         ["Recently-failed-job check errored"] = "reads the monitored server's msdb on its own connection and timeout",
@@ -1191,8 +1192,11 @@ WebTlsCertificateState report the web host publishes - there is no store read to
            re-announcement at the next restart and never a delivery. 26th since #3915: the store-log re-mask
            pass, a maintenance rewrite of stored text that resumes next hour, with the reader masking in the
            meantime. 27th since #3908: the store TimescaleDB self-alert's catch, whose evidence is the version
-           the bootstrap read before the alert engine existed. */
-        Assert.Equal(27, totalExempt);
+           the bootstrap read before the alert engine existed. 28th since #3923: the sizing pass's own narrow
+           catch, mirroring the store-log capture's catch beside it - a metrics write sweep whose swallowed
+           failure costs no alert, and whose whole point is to leave the store-log census and the
+           collector-cost flush below it running on the connection this catch keeps open. */
+        Assert.Equal(28, totalExempt);
 
         /* Every exemption in the table is actually used. An exemption for a message that no longer exists
            is a hole this pin would otherwise keep open indefinitely — the shape that lets a real new catch

Darling/Darling.Tests/StoreLogSelfMonitoringStoreTests.cs

@@ -267,13 +267,35 @@ public void TheSweepRidesTheHourlySelfMetricsTick()
         Assert.Contains("StoreSelfMetrics.SweepAsync", body, StringComparison.Ordinal);
         Assert.Contains("StoreLogSweep.SweepAsync", body, StringComparison.Ordinal);
 
-        /* And it has its OWN catch, which the two passes either side of it do not. This is the only pass
-           on the tick whose PRIVILEGE is not guaranteed - reading the log needs pg_read_server_files plus
-           EXECUTE on pg_read_binary_file, which a bring-your-own store's owner may not have granted - so
-           sharing the outer catch would let one permanent condition cost the collector-cost flush BELOW it
-           every hour forever. Asserted by position: a catch has to sit between the two calls. */
+        var sizingSweep = body.IndexOf("StoreSelfMetrics.SweepAsync", StringComparison.Ordinal);
         var logSweep = body.IndexOf("StoreLogSweep.SweepAsync", StringComparison.Ordinal);
         var costFlush = body.IndexOf("_collectorCost.FlushAsync", StringComparison.Ordinal);
+
+        /* #3923: the sizing pass gets its OWN narrow catch too, ahead of the store-log capture - the same
+           "cost the flush below it forever" reasoning the capture's catch already carries, hit for real by
+           #3918 (a catch-all census failing 42703 on TimescaleDB 2.29+) while the sizing pass still shared
+           the OUTER catch. Position: between the sizing call and the store-log capture, asserted the same
+           way as the capture's own catch below - a mutation that drops it, or moves it past the capture,
+           reds here. Filter: excludes a command TIMEOUT (still the outer catch's job) and a connection the
+           fault already broke (the two passes after it need it live) - the two conditions checked directly
+           against the catch's own text so a mutation that widens either arm reds too. */
+        var sizingCatch = body.IndexOf("catch (PostgresException ex) when", StringComparison.Ordinal);
+        Assert.True(
+            sizingCatch > sizingSweep && sizingCatch < logSweep,
+            "the sizing pass has no catch of its own between it and the store-log capture, so a statement "
+            + "the store permanently rejects there would cost the store-log census and the collector-cost "
+            + "flush too");
+
+        var sizingCatchBody = body[sizingCatch..logSweep];
+        Assert.Contains("!PgBaselineProvider.IsCommandTimeout(ex)", sizingCatchBody, StringComparison.Ordinal);
+        Assert.Contains("connection.State == ConnectionState.Open", sizingCatchBody, StringComparison.Ordinal);
+
+        /* And the store-log capture has its OWN catch, which the two passes either side of it do not. This
+           is the only pass on the tick whose PRIVILEGE is not guaranteed - reading the log needs
+           pg_read_server_files plus EXECUTE on pg_read_binary_file, which a bring-your-own store's owner may
+           not have granted - so sharing the outer catch would let one permanent condition cost the
+           collector-cost flush BELOW it every hour forever. Asserted by position: a catch has to sit between
+           the two calls. */
         var innerCatch = body.IndexOf("catch (Exception ex) when", logSweep, StringComparison.Ordinal);
 
         Assert.True(costFlush > logSweep, "the collector-cost flush no longer follows the store-log sweep");
@@ -282,6 +304,25 @@ on the tick whose PRIVILEGE is not guaranteed - reading the log needs pg_read_se
             "the store-log sweep has no catch of its own between it and the collector-cost flush, so a "
             + "store whose role cannot read the log would cost the flush too");
 
+        /* #3971: the passes after that catch need a connection it may have just closed - Npgsql closes THIS
+           one outright on the capture's ERROR rather than leaving it idle-but-usable. Position: the reopen
+           has to sit between the store-log catch and the collector-cost flush, so the re-mask in between
+           (which runs on the same connection) gets it live too. */
+        var reopen = body.IndexOf("connection.State != ConnectionState.Open", StringComparison.Ordinal);
+        Assert.True(
+            reopen > innerCatch && reopen < costFlush,
+            "the connection is never checked and reopened between the store-log catch and the "
+            + "collector-cost flush, so a capture that closed it would take the re-mask and the flush "
+            + "down too");
+
+        /* #3971: a capture the store will never allow (58P01 no log directory, 42501 no log read) warns once
+           per process, then drops to Debug, inside the capture's own catch. */
+        var captureCatch = body.Substring(innerCatch, reopen - innerCatch);
+        Assert.Contains("PostgresErrorCodes.UndefinedFile", captureCatch, StringComparison.Ordinal);
+        Assert.Contains("PostgresErrorCodes.InsufficientPrivilege", captureCatch, StringComparison.Ordinal);
+        Assert.Contains("_storeLogCaptureUnavailableWarned", captureCatch, StringComparison.Ordinal);
+        Assert.Contains("LogDebug(", captureCatch, StringComparison.Ordinal);
+
         /* No second timer: a new cadence field would put the two series on different grids, which is the
            thing riding this tick buys. */
         Assert.DoesNotContain("_nextStoreLog", worker, StringComparison.Ordinal);

Darling/Darling.Tests/StoreSelfMetricsTickRecoveryLiveTests.cs

@@ -0,0 +1,105 @@
+/*
+ * 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.Data;
+using System.Threading.Tasks;
+using Microsoft.Extensions.Logging.Abstractions;
+using Npgsql;
+using PerformanceMonitor.Darling.Service;
+using PerformanceMonitor.Darling.Storage;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #3971: a store with no readable server-log directory does not lose the hourly collector-cost flush.
+///
+/// <para><b>The reproduction, against a real store.</b> A fresh PostgreSQL cluster with
+/// <c>logging_collector</c> off (the default, and the Linux compose store's shape) has no <c>log</c>
+/// directory at all — <c>pg_ls_logdir()</c>, which <see cref="StoreLogSweep.SweepAsync"/> lists the capture
+/// candidates through, fails with <c>58P01 could not open directory "log"</c> before this test does anything
+/// to force it. And Npgsql does not leave the connection idle-but-usable after that ERROR: it closes it
+/// outright, which is the fact <c>DarlingWorker.SweepStoreSelfMetricsAsync</c>'s next two passes — the
+/// store-log re-mask (#3915) and the collector-cost flush (#2674) — ran into every hour, on the same
+/// connection, before the fix beside this test.</para>
+///
+/// <para><b>Why this test does not call <c>SweepStoreSelfMetricsAsync</c> itself.</b> That method is a
+/// private instance member of a <c>BackgroundService</c> with a full worker's worth of DI behind it, so —
+/// like the re-mask pass beside it (<see cref="StoreLogRemaskLiveTests"/>) — the live behaviour is proven
+/// against the real primitives it calls (<see cref="StoreLogSweep.SweepAsync"/> and
+/// <see cref="CollectorCostAccumulator.FlushAsync"/>) on one connection, in the same order, while
+/// <c>StoreLogSelfMonitoringStoreTests.TheSweepRidesTheHourlySelfMetricsTick</c> pins that the worker method
+/// actually wires the reopen between them.</para>
+///
+/// <para><b>#1776 own-store</b>: it mints and migrates a scratch database through <c>ScratchPostgres</c>, so
+/// it is not in the <c>live-postgres</c> collection.</para>
+/// </summary>
+public sealed class StoreSelfMetricsTickRecoveryLiveTests
+{
+    [Fact]
+    public async Task AFailedStoreLogCapture_ClosesTheConnection_AndReopeningItLetsTheCollectorCostFlushWrite()
+    {
+        var baseConnectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG");
+        Assert.SkipWhen(string.IsNullOrWhiteSpace(baseConnectionString),
+            "Set DARLING_TEST_PG to a superuser connection string to run the #3971 capture-recovery proof "
+            + "in a scratch database.");
+
+        var ct = TestContext.Current.CancellationToken;
+        await using var scratch = await ScratchPostgres.CreateAsync(baseConnectionString!, ct);
+        await using var c = new NpgsqlConnection(scratch.ConnectionString);
+        await c.OpenAsync(ct);
+        await PgMigrations.MigrateAsync(c, ct);
+
+        /* The reproduction. No ALTER SYSTEM needed: logging_collector defaults to off, so a fresh cluster
+           never creates its log directory, which is exactly the Linux compose store's shape (#3971's own
+           evidence) rather than a condition this test manufactures. */
+        var capture = await Assert.ThrowsAsync<PostgresException>(
+            () => StoreLogSweep.SweepAsync(c, DateTime.UtcNow, NullLogger.Instance, ct));
+        Assert.Equal("58P01", capture.SqlState);
+        Assert.Contains("could not open directory", capture.Message, StringComparison.Ordinal);
+
+        /* #3971's central claim: Npgsql does not leave this connection idle-but-usable after that ERROR. If
+           this ever reads Open on a future Npgsql/PostgreSQL pairing, the reopen this test and the worker
+           both do below becomes a no-op, not a wrong fix — but the claim itself needs to keep being true, or
+           the rest of this test proves nothing. */
+        Assert.Equal(ConnectionState.Closed, c.State);
+
+        /* The fix (DarlingWorker.SweepStoreSelfMetricsAsync carries the same check): reopen before anything
+           else on this connection runs. Disable this block (`if (false && ...)`) and the very next
+           assertion reds with Expected Open / Actual Closed; remove that assertion too and the run reaches
+           the collector-cost flush below and throws InvalidOperationException: "Connection is not open" —
+           the exact failure #3971 reported for the re-mask and the collector-cost flush. Both are how this
+           test was proven to fail on the code before this PR. */
+        if (c.State != ConnectionState.Open)
+        {
+            await c.OpenAsync(ct);
+        }
+
+        Assert.Equal(ConnectionState.Open, c.State);
+
+        /* The collector-cost flush still writes its row on the reopened connection — the outcome #3971 says
+           was lost every hour on a store shaped like this one. */
+        var accumulator = new CollectorCostAccumulator();
+        accumulator.Record(serverId: 397_100, collectorName: "test_collector_3971", rows: 10, sqlMs: 5, storageMs: 2);
+        await accumulator.FlushAsync(c, DateTime.UtcNow, NullLogger.Instance, ct);
+
+        await using var read = new NpgsqlCommand(
+            "SELECT run_count, total_sql_ms, max_sql_ms, total_storage_ms, total_rows "
+            + "FROM collect.collector_cost WHERE server_id = 397100 AND collector_name = 'test_collector_3971'",
+            c);
+        await using var reader = await read.ExecuteReaderAsync(ct);
+        Assert.True(await reader.ReadAsync(ct), "the collector-cost flush wrote no row for the reopened connection");
+        Assert.Equal(1, reader.GetInt32(0));
+        Assert.Equal(5L, reader.GetInt64(1));
+        Assert.Equal(5L, reader.GetInt64(2));
+        Assert.Equal(2L, reader.GetInt64(3));
+        Assert.Equal(10L, reader.GetInt64(4));
+        Assert.False(await reader.ReadAsync(ct), "more than one row landed for one Record call");
+    }
+}