Deadlock re-mask keys a missed alert instead of leaving its raw hash, walks pages to its hourly budget, and a failing stage no longer blocks the rest (#4012)

Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs

@@ -1017,7 +1017,7 @@ private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScop
         ["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",
-        ["PostgreSQL deadlocks: re-masking alerts and reports stored before this build failed"] = "a maintenance rewrite of stored deadlock reports and deadlock-alert history (#4012); no alert is judged on it, it rewrites only rows still raw and resumes next hour, and until it finishes every deadlock read normalizes the rows on the way out",
+        ["PostgreSQL deadlocks: re-masking alerts, reports and findings stored before this build failed"] = "a maintenance rewrite of stored deadlock reports, deadlock-alert history and analysis findings (#4012); no alert is judged on it, it rewrites only rows still raw and resumes next hour (each stage counts and caps its own failures, #4012's review), and until it finishes every deadlock read normalizes the rows on the way out",
         ["Recently-failed-job check errored"] = "reads the monitored server's msdb on its own connection and timeout",
         ["Skipping recently-failed-job check"] = "the same msdb read, permission-denied arm; not a store read",
         ["Failed to check failed jobs"] = "the fetcher reads the monitored server's msdb; the block's only store op is a write both stores swallow",

Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs

@@ -656,6 +656,10 @@ what Lite's twin and every pre-#3477 test constructs. */
 
     private readonly PgLogHashKey? _logHashKey;
 
+    /// <summary>The store's log-hash key this runner was given (#4004), null when the service could not load one. The
+    /// hourly deadlock re-mask reads it here (#4012's review), the one instance every log-hashing run shares.</summary>
+    internal PgLogHashKey? LogHashKey => _logHashKey;
+
     /* One ingestor for the process, so the resume marker survives between cycles - it is per-file and
        in-memory by design (#2538), and a fresh instance every cycle would silently re-read the same tail
        forever while looking like it was making progress. */

Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs

@@ -854,30 +854,16 @@ flip mid-run is picked up by each of them on its next pass with no further wirin
 
     private bool _storeLogRemaskDone;
 
-    /* #4012: the re-mask of PostgreSQL deadlock alerts, then stored analysis findings' deadlock exemplars, then
-       deadlock reports, stored before #4005 with their SQL raw. The alerts go first because they are found through the reports' raw hashes, which the report pass
-       replaces. Each has a cursor and is done once a slice reaches its table's end; a scan that left rows raw
-       (a row that changed under its write, a report whose rewrite failed) reads its table through again, at
-       most PgDeadlockRemask.MaxRescansPerProcess times, and the next process's pass is a no-op re-read. */
-    private string? _pgDeadlockAlertRemaskCursor;
+    /* #4012: the re-mask of PostgreSQL deadlock alerts, then deadlock reports, then stored analysis findings'
+       deadlock exemplars, stored before #4005 with their SQL raw. The alerts go first because they are found
+       through the reports' raw hashes, which the report stage replaces; the findings last, so their read cannot
+       hold the reports back. In memory, like the store-log cursor above (#4012's review ruled out a store
+       migration): each stage is done after a whole walk that rewrote nothing, and a restart walks again. */
+    private readonly PgDeadlockRemask.RemaskProgress _pgDeadlockRemask = new();
 
-    private bool _pgDeadlockAlertRemaskLeftRows;
-
-    private bool _pgDeadlockAlertRemaskDone;
-
-    private string? _pgDeadlockFindingRemaskCursor;
-
-    private bool _pgDeadlockFindingRemaskLeftRows;
-
-    private bool _pgDeadlockFindingRemaskDone;
-
-    private PgDeadlockRemask.ReportCursor? _pgDeadlockRemaskCursor;
-
-    private bool _pgDeadlockRemaskLeftRows;
-
-    private bool _pgDeadlockRemaskDone;
-
-    private int _pgDeadlockRemaskRescans;
+    /* #4012: the re-mask keys an alert whose report is gone under the store's log-hash key. Null when the service
+       has none; every other stage runs without it (#4012's review, finding 1). */
+    private PgLogHashKey? _pgDeadlockRemaskKey;
 
     /* #3971: a store-log capture that fails for a reason the store will not change on its own warns once per
        process, then logs at Debug: 58P01, no log directory (the Linux compose store logs to stderr and has
@@ -2866,6 +2852,10 @@ server that cannot be reached is exactly the kind of fact it must carry (its sto
             if (DateTime.UtcNow >= _nextStoreMetricsUtc)
             {
                 _nextStoreMetricsUtc = DateTime.UtcNow.Add(s_storeMetricsInterval);
+
+                /* #4012's review: the deadlock re-mask inside the sweep keys an alert whose report is gone under the
+                   same log-hash key the runner's log-event runs share. */
+                _pgDeadlockRemaskKey = runner.LogHashKey;
                 await SweepStoreSelfMetricsAsync(stoppingToken);
 
                 /* #2674: right after the flush wrote the latest hour, evaluate whether any of our collectors
@@ -7013,87 +7003,36 @@ slice may cost the collector-cost flush below. */
 
             /* #4012: PostgreSQL deadlock reports stored before #4005 keep their SQL raw, with a hash over the raw
                graph, for pg_deadlocks' 90 days, and a deadlock alert fired before it keeps the same in its history
-               row, and an analysis finding its exemplars. Every read normalizes them; a direct SELECT does not. One
-               bounded slice per tick, the alerts' until they are done, then the findings', then the reports', with the store-log slice's own-catch, own-cap posture:
-               neither a failure nor a slow slice may cost the collector-cost flush below. */
-            if (!_pgDeadlockRemaskDone)
+               row, and an analysis finding its exemplars. Every read normalizes them; a direct SELECT does not.
+               Page after page until the slice's own cap (#4012's review: a page an hour took about 11 days for the
+               reports alone), alerts, then reports, then findings, with the store-log slice's own-catch, own-cap
+               posture: neither a failure nor a slow slice may cost the collector-cost flush below. RunAsync ends
+               quietly on its cap and keeps each walk's cursor; a stage's own failures are counted and logged there.
+               Gated on Pending, not Done (#4036's round-2 review, finding 1): a stage that gave up counts as done,
+               and RunAsync is where it is tried again a day later, so it must still be called. */
+            if (_pgDeadlockRemask.Pending)
             {
                 if (connection.State != ConnectionState.Open)
                 {
                     await connection.CloseAsync();
                     await connection.OpenAsync(budget.Token);
                 }
 
+                /* #4012's review, finding 1: no key is no reason to skip the pass. The key file can be unusable for
+                   good (an untrusted directory or ACL, an unreadable file, DPAPI after the service moved machines),
+                   and the reports, alerts, findings and finding alerts need no key; only the alert-key stage waits
+                   for one, and RunAsync says so once. */
                 using var remaskBudget = CancellationTokenSource.CreateLinkedTokenSource(budget.Token);
                 remaskBudget.CancelAfter(PgDeadlockRemask.SliceBudget);
                 try
                 {
-                    if (!_pgDeadlockAlertRemaskDone)
-                    {
-                        var (next, examined, rewritten, raced) = await PgDeadlockRemask.RemaskStoredAlertsAsync(
-                            connection, _pgDeadlockAlertRemaskCursor, remaskBudget.Token);
-                        _pgDeadlockAlertRemaskCursor = next;
-                        _pgDeadlockAlertRemaskLeftRows |= raced > 0;
-                        if (next is null)
-                        {
-                            _pgDeadlockAlertRemaskDone = !_pgDeadlockAlertRemaskLeftRows
-                                || ++_pgDeadlockRemaskRescans > PgDeadlockRemask.MaxRescansPerProcess;
-                            _pgDeadlockAlertRemaskLeftRows = false;
-                        }
-
-                        if (rewritten > 0)
-                        {
-                            _logger.LogInformation(
-                                "PostgreSQL deadlocks: re-masked {Rewritten} of {Examined} deadlock alert(s) fired before this build, so their SQL literals and raw report hashes no longer sit in the alert history{Remaining}.",
-                                rewritten, examined, next is null ? "" : "; the rest follow on the next hourly passes");
-                        }
-                    }
-                    else if (!_pgDeadlockFindingRemaskDone)
-                    {
-                        var (next, examined, rewritten, raced) = await PgDeadlockRemask.RemaskStoredFindingsAsync(
-                            connection, _pgDeadlockFindingRemaskCursor, remaskBudget.Token);
-                        _pgDeadlockFindingRemaskCursor = next;
-                        _pgDeadlockFindingRemaskLeftRows |= raced > 0;
-                        if (next is null)
-                        {
-                            _pgDeadlockFindingRemaskDone = !_pgDeadlockFindingRemaskLeftRows
-                                || ++_pgDeadlockRemaskRescans > PgDeadlockRemask.MaxRescansPerProcess;
-                            _pgDeadlockFindingRemaskLeftRows = false;
-                        }
-
-                        if (rewritten > 0)
-                        {
-                            _logger.LogInformation(
-                                "PostgreSQL deadlocks: re-masked {Rewritten} of {Examined} stored analysis finding(s) written before this build, so their deadlock exemplars' SQL literals no longer sit in the findings store{Remaining}.",
-                                rewritten, examined, next is null ? "" : "; the rest follow on the next hourly passes");
-                        }
-                    }
-                    else
-                    {
-                        var (next, examined, rewritten, failed) = await PgDeadlockRemask.RemaskStoredReportsAsync(
-                            connection, _pgDeadlockRemaskCursor, remaskBudget.Token);
-                        _pgDeadlockRemaskCursor = next;
-                        _pgDeadlockRemaskLeftRows |= failed > 0;
-                        if (next is null)
-                        {
-                            _pgDeadlockRemaskDone = !_pgDeadlockRemaskLeftRows
-                                || ++_pgDeadlockRemaskRescans > PgDeadlockRemask.MaxRescansPerProcess;
-                            _pgDeadlockRemaskLeftRows = false;
-                        }
-
-                        if (rewritten > 0 || failed > 0)
-                        {
-                            _logger.LogInformation(
-                                "PostgreSQL deadlocks: re-masked {Rewritten} stored report row(s) of {Examined} examined, captured before this build, so their SQL literals and raw report hashes no longer sit in pg_deadlocks; {Failed} report(s) could not be rewritten and are retried{Remaining}.",
-                                rewritten, examined, failed, next is null ? "" : "; the rest follow on the next hourly passes");
-                        }
-                    }
+                    await PgDeadlockRemask.RunAsync(connection, _pgDeadlockRemask, _pgDeadlockRemaskKey, _logger, remaskBudget.Token);
                 }
                 catch (Exception ex) when (ex is not OperationCanceledException
                     || (remaskBudget.IsCancellationRequested && !budget.IsCancellationRequested))
                 {
                     _logger.LogWarning(
-                        "PostgreSQL deadlocks: re-masking alerts and reports stored before this build failed, and is retried next hour: {Message}",
+                        "PostgreSQL deadlocks: re-masking alerts, reports and findings stored before this build failed, and is retried next hour: {Message}",
                         ex.Message);
                 }
             }