diff --git a/Darling/Darling.Tests/AlertEngineTests.cs b/Darling/Darling.Tests/AlertEngineTests.cs index adb0c831d..c574ef65b 100644 --- a/Darling/Darling.Tests/AlertEngineTests.cs +++ b/Darling/Darling.Tests/AlertEngineTests.cs @@ -187,6 +187,33 @@ public Task> GetForcePlanFailuresAsync(string serverK } } + /// A state store whose persistence SAVE always throws — everything else behaves. #3282's + /// degradation path: the gate must keep working from its in-memory record. + private sealed class ThrowingPersistenceSaveStore : IAlertStateStore + { + private readonly FakeStateStore _inner = new(); + + public int SaveAttempts { get; private set; } + + public Task LoadEdgeTriggerWatermarkAsync(string serverKey, string metricName) => _inner.LoadEdgeTriggerWatermarkAsync(serverKey, metricName); + public Task SaveEdgeTriggerWatermarkAsync(string serverKey, string metricName, int watermark) => _inner.SaveEdgeTriggerWatermarkAsync(serverKey, metricName, watermark); + public Task LoadFailedJobWatermarkAsync(string serverKey) => _inner.LoadFailedJobWatermarkAsync(serverKey); + public Task SaveFailedJobWatermarkAsync(string serverKey, DateTime watermark) => _inner.SaveFailedJobWatermarkAsync(serverKey, watermark); + public Task SaveDatabaseStateAlertedAsync(string serverKey, string databaseName, string effectiveState) => _inner.SaveDatabaseStateAlertedAsync(serverKey, databaseName, effectiveState); + public Task ClearDatabaseStateAlertedAsync(string serverKey, string databaseName) => _inner.ClearDatabaseStateAlertedAsync(serverKey, databaseName); + public Task> LoadIncidentOccurrencesAsync(string serverKey, string metricName) => _inner.LoadIncidentOccurrencesAsync(serverKey, metricName); + public Task SaveIncidentOccurrencesAsync(string serverKey, string metricName, IReadOnlyDictionary states) => _inner.SaveIncidentOccurrencesAsync(serverKey, metricName, states); + + public Task LoadAlertPersistenceAsync(string serverKey, string metricName) => + Task.FromResult(null); + + public Task SaveAlertPersistenceAsync(string serverKey, string metricName, AlertPersistenceRecord record) + { + SaveAttempts++; + throw new InvalidOperationException("persistence store is down"); + } + } + private sealed class FakeStateStore : IAlertStateStore { public Dictionary<(string Key, string Metric), int> EdgeWatermarks { get; } = new(); @@ -225,6 +252,21 @@ public Task> LoadIncidentOc ? states : new Dictionary(StringComparer.Ordinal)); + + /* #3282: REAL persistence, not a no-op — the gate's whole point is that it survives a restart, and + a fake that forgot the record would let a broken seed path pass. Keyed like both real stores. */ + public Dictionary<(string Key, string Metric), AlertPersistenceRecord> Persistence { get; } = new(); + public List<(string Key, string Metric, AlertPersistenceRecord Record)> SavedPersistence { get; } = new(); + + public Task LoadAlertPersistenceAsync(string serverKey, string metricName) => + Task.FromResult(Persistence.TryGetValue((serverKey, metricName), out var r) ? (AlertPersistenceRecord?)r : null); + + public Task SaveAlertPersistenceAsync(string serverKey, string metricName, AlertPersistenceRecord record) + { + Persistence[(serverKey, metricName)] = record; + SavedPersistence.Add((serverKey, metricName, record)); + return Task.CompletedTask; + } public Task SaveIncidentOccurrencesAsync(string serverKey, string metricName, IReadOnlyDictionary states) { /* Replace-the-set, exactly like both real stores: whatever arrives IS the metric's state, so an @@ -306,10 +348,26 @@ into the process-wide AlertReadFailureCounter.Shared. */ utcNow: () => Now, readFailures: ReadFailures); + /* #3282: every snapshot gets a DISTINCT, increasing CPU sample instant unless the test says + otherwise, because that is the realistic case — the sweep sees a new ring-buffer sample each + time — and because the alternative default would be dishonest in both directions. A fixed + instant would make every sweep a stale re-read and silently freeze the CPU gate in tests that + are not about the gate; a null would put them on the no-sample-instant DEGRADATION path while + reading like the normal one. Tests that want a stale re-read pass the same instant twice, and + the one test about the null path passes it explicitly. */ + private static int s_sampleTick; + public static AlertServerSnapshot Snapshot( double? sqlCpu = null, double? totalCpu = null, - bool isOnline = true, bool isAzureSqlDb = false, bool suppressed = false) => - new(Key, Name, isOnline, sqlCpu, totalCpu, isAzureSqlDb, suppressed); + bool isOnline = true, bool isAzureSqlDb = false, bool suppressed = false, + DateTime? cpuSampleTime = null, bool noCpuSampleTime = false) => + new(Key, Name, isOnline, sqlCpu, totalCpu, isAzureSqlDb, suppressed, + noCpuSampleTime + ? null + : cpuSampleTime ?? SampleBase.AddMinutes(System.Threading.Interlocked.Increment(ref s_sampleTick))); + + /// The instant distinct sample times are counted from — arbitrary, only the ordering matters. + public static readonly DateTime SampleBase = new(2026, 7, 1, 0, 0, 0, DateTimeKind.Utc); } private static BlockedProcessAlertRow BlockingRow( @@ -352,17 +410,57 @@ public async Task AlertsDisabled_RunsNoChecksAtAll() Assert.Equal(0, h.Adapter.BlockingFetches); } - /* ---------------- CPU ---------------- */ + /* ---------------- CPU: the #3282 persistence gate ---------------- */ + + /// + /// Drives consecutive sweeps, each carrying a DISTINCT and increasing CPU + /// sample instant — one gate observation per call, which is what the gate counts. Returns the instant + /// of the last sample so a caller can keep the sequence going. + /// + private static async Task DriveCpuAsync( + AlertEngine engine, double? sqlCpu, double? totalCpu, int samples, + DateTime from, bool suppressed = false) + { + var at = from; + for (var i = 0; i < samples; i++) + { + at = at.AddMinutes(1); + await engine.EvaluateServerAsync( + Harness.Snapshot(sqlCpu: sqlCpu, totalCpu: totalCpu, suppressed: suppressed, cpuSampleTime: at)); + } + + return at; + } + + [Fact] + public async Task Cpu_OneSampleOverTheBar_DoesNotFire() + { + /* THE #3282 defect, stated as a pin: a single sample over the threshold used to be an incident. + This is the assertion that reddens if CpuBreachSamples goes back to 1. */ + var h = new Harness(); + h.Settings.CpuEnabled = true; + var engine = h.Build(); + + await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 99, samples: 1, from: Harness.SampleBase); + + Assert.Empty(h.Deliverer.Outcomes); + Assert.Empty(h.Resolutions); + } [Fact] - public async Task Cpu_FiresAtThresholdInclusive_ThenCooldownSuppressesRepeat() + public async Task Cpu_FiresOnlyOnTheThirdConsecutiveBreachingSample_ThenCooldownSuppressesRepeat() { - /* Lite AlertEngine.cs:65-67 (>= threshold) and :72 (cooldown gates the repeat). */ + /* Lite AlertEngine.cs:65-67 (>= threshold) and :72 (cooldown gates the repeat), now behind the + gate: the threshold comparison and the delivered shape are UNCHANGED, only the number of + samples it takes to get there. */ var h = new Harness(); h.Settings.CpuEnabled = true; var engine = h.Build(); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 80)); + var at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 80, samples: AlertEngine.CpuBreachSamples - 1, from: Harness.SampleBase); + Assert.Empty(h.Deliverer.Outcomes); + + at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 80, samples: 1, from: at); var fired = Assert.Single(h.Deliverer.Outcomes); Assert.Equal("High CPU", fired.MetricName); Assert.Equal("80% (Total CPU)", fired.CurrentValue); /* :82 current-value shape, :64 label */ @@ -376,85 +474,339 @@ public async Task Cpu_FiresAtThresholdInclusive_ThenCooldownSuppressesRepeat() /* Same breach 1 minute later: inside the 5-minute cooldown — no repeat (:72). */ h.Now = h.Now.AddMinutes(1); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 85)); + at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 85, samples: 1, from: at); Assert.Single(h.Deliverer.Outcomes); - /* After the cooldown elapses the standing breach re-fires (CPU is level-triggered). */ + /* After the cooldown elapses the STANDING breach re-fires. #3282 changed what counts as an + incident, deliberately not how often a standing one repeats: the gate's rising edge is a + separate question from the reminder cadence, and silencing the reminder would be a second + behaviour change nobody asked for. */ h.Now = h.Now.AddMinutes(5); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 85)); + await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 85, samples: 1, from: at); Assert.Equal(2, h.Deliverer.Outcomes.Count); } + [Fact] + public async Task Cpu_AStreakBrokenByOneClearSample_NeverFires() + { + /* Two breaches, one sample under the bar, then two more breaches: four breaching samples in all + and never three in a ROW, so nothing fires. The reset is what makes "sustained" mean sustained + rather than "often". */ + var h = new Harness(); + h.Settings.CpuEnabled = true; + var engine = h.Build(); + + var at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 90, samples: 2, from: Harness.SampleBase); + at = await DriveCpuAsync(engine, sqlCpu: 20, totalCpu: 40, samples: 1, from: at); + await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 90, samples: 2, from: at); + + Assert.Empty(h.Deliverer.Outcomes); + } + + [Fact] + public async Task Cpu_RepeatedSampleInstant_DoesNotAdvanceTheStreak() + { + /* The reason the gate counts SAMPLES and not sweeps. The alert sweep is 30 seconds and a CPU + sample advances about a minute, so the sweep re-reads the same row roughly every other pass. + Here the SAME instant is offered CpuBreachSamples x 3 times: one observation, no fire. + + Without the freshness check this test fires, and with it the measured excursions stay + suppressed — that is the whole difference between fixing #3282 and adding 90 seconds of + latency to it. */ + var h = new Harness(); + h.Settings.CpuEnabled = true; + var engine = h.Build(); + + var stuck = Harness.SampleBase.AddMinutes(1); + for (var i = 0; i < AlertEngine.CpuBreachSamples * 3; i++) + { + await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 95, cpuSampleTime: stuck)); + } + + Assert.Empty(h.Deliverer.Outcomes); + + /* Two genuinely new samples on top of that one observation reach the bar. */ + await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 95, samples: AlertEngine.CpuBreachSamples - 1, from: stuck); + Assert.Single(h.Deliverer.Outcomes); + } + + [Fact] + public async Task Cpu_TheMeasuredTwoMinuteExcursion_IsNotAnIncident() + { + /* A replay of the worst real excursion behind #3282, one minute per stored sample: total CPU + 39 → 68 → 73 → 93 → 93 → 53 → 55 → 21 against the 80% default. Two consecutive samples at or + above the bar, then back to a ~20% baseline. The pre-gate engine delivered a High CPU at 93% + and a CPU Resolved at 55% about 147 seconds apart; both are noise, and neither should appear. */ + var h = new Harness(); + h.Settings.CpuEnabled = true; + var engine = h.Build(); + + var at = Harness.SampleBase; + foreach (var total in new double[] { 39, 68, 73, 93, 93, 53, 55, 21 }) + { + at = at.AddMinutes(1); + await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: total - 3, totalCpu: total, cpuSampleTime: at)); + } + + Assert.Empty(h.Deliverer.Outcomes); + Assert.Empty(h.Resolutions); + } + [Fact] public async Task Cpu_ModeSelection_HappensInsideTheEngine() { /* CpuPercentForAlert semantics (Lite LocalDataService.Overview.cs:143-144): - Total → TotalCpuPercent ?? CpuPercent; SqlOnly → CpuPercent. */ + Total → TotalCpuPercent ?? CpuPercent; SqlOnly → CpuPercent. Unchanged by #3282 — the gate + sits after the mode selection, so each engine here is driven CpuBreachSamples times. */ var h = new Harness(); h.Settings.CpuEnabled = true; + /* The sample instant is threaded FORWARD across the sub-cases rather than restarted, because every + h.Build() here shares one state store: a re-used instant is a sample the gate has already + counted, so restarting the sequence silently freezes it and each sub-case would assert against + an engine that observed nothing. (Found by running these, not by reading them.) */ + var at = Harness.SampleBase; + /* SqlProcess mode compares the SQL value even when total is higher. */ h.Settings.CpuAlertMode = CpuAlertMode.SqlProcess; - await h.Build().EvaluateServerAsync(Harness.Snapshot(sqlCpu: 50, totalCpu: 95)); + at = await DriveCpuAsync(h.Build(), sqlCpu: 50, totalCpu: 95, samples: AlertEngine.CpuBreachSamples, from: at); Assert.Empty(h.Deliverer.Outcomes); - await h.Build().EvaluateServerAsync(Harness.Snapshot(sqlCpu: 85, totalCpu: 95)); + at = await DriveCpuAsync(h.Build(), sqlCpu: 85, totalCpu: 95, samples: AlertEngine.CpuBreachSamples, from: at); Assert.Equal("85% (SQL CPU)", Assert.Single(h.Deliverer.Outcomes).CurrentValue); - /* TotalServer mode falls back to the SQL value when no total is available. */ - h.Deliverer.Outcomes.Clear(); - h.Settings.CpuAlertMode = CpuAlertMode.TotalServer; - await h.Build().EvaluateServerAsync(Harness.Snapshot(sqlCpu: 90, totalCpu: null)); + /* TotalServer mode falls back to the SQL value when no total is available. A fresh harness, so the + incident the sub-case above opened cannot mask this one's rising edge. */ + h = new Harness { Settings = { CpuEnabled = true, CpuAlertMode = CpuAlertMode.TotalServer } }; + at = await DriveCpuAsync(h.Build(), sqlCpu: 90, totalCpu: null, samples: AlertEngine.CpuBreachSamples, from: at); Assert.Equal("90% (Total CPU)", Assert.Single(h.Deliverer.Outcomes).CurrentValue); - /* No CPU sample at all → no alert (:66 HasValue gate). */ - h.Deliverer.Outcomes.Clear(); - await h.Build().EvaluateServerAsync(Harness.Snapshot(sqlCpu: null, totalCpu: null)); + /* No CPU sample at all → no alert (:66 HasValue gate), on a fresh harness so nothing is open. */ + h = new Harness { Settings = { CpuEnabled = true, CpuAlertMode = CpuAlertMode.TotalServer } }; + await DriveCpuAsync(h.Build(), sqlCpu: null, totalCpu: null, samples: AlertEngine.CpuBreachSamples, from: at); Assert.Empty(h.Deliverer.Outcomes); } [Fact] - public async Task Cpu_RecoveryEmitsResolution_WithLiteToastStrings_UnlessSuppressed() + public async Task Cpu_ResolvesOnlyAfterTwoConsecutiveClearSamples_WithLiteToastStrings_UnlessSuppressed() { - /* Lite AlertEngine.cs:101-113 — active→inactive announces "CPU Resolved" gated on - !suppressPopups && enabled (:107). */ + /* Lite AlertEngine.cs:101-113 — the "CPU Resolved" strings are unchanged; what changed is that + ONE sample under the bar no longer announces a recovery. */ var h = new Harness(); h.Settings.CpuEnabled = true; var engine = h.Build(); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 90)); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 20, totalCpu: 40)); + var at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 90, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase); + Assert.Single(h.Deliverer.Outcomes); + + at = await DriveCpuAsync(engine, sqlCpu: 20, totalCpu: 40, samples: AlertEngine.CpuClearSamples - 1, from: at); + Assert.Empty(h.Resolutions); + at = await DriveCpuAsync(engine, sqlCpu: 20, totalCpu: 40, samples: 1, from: at); var resolution = Assert.Single(h.Resolutions); Assert.Equal("CPU Resolved", resolution.Title); /* :110 */ Assert.Equal("SRV-A: Total CPU back to 40%", resolution.Message); /* :111 */ Assert.Equal("High CPU", resolution.MetricName); - /* Suppressed recovery still flips the active state but says nothing (:107). */ + /* Suppressed: the gate still advances (suppression is evaluate-but-don't-deliver) and says + nothing (:107). */ h.Resolutions.Clear(); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 90, suppressed: true)); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 20, totalCpu: 40, suppressed: true)); + h.Deliverer.Outcomes.Clear(); + at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 90, samples: AlertEngine.CpuBreachSamples, from: at, suppressed: true); + await DriveCpuAsync(engine, sqlCpu: 20, totalCpu: 40, samples: AlertEngine.CpuClearSamples, from: at, suppressed: true); Assert.Empty(h.Resolutions); + Assert.Empty(h.Deliverer.Outcomes); } [Fact] - public async Task Cpu_Suppressed_SetsActiveButDoesNotDeliverOrStampCooldown() + public async Task Cpu_ABreachBeforeTheClearThreshold_KeepsTheIncidentOpen() { - /* Lite AlertEngine.cs:71-72 — active is recorded, but the !suppressPopups gate sits - BEFORE the cooldown stamp, so nothing is delivered and nothing is stamped. */ + /* The other half of the resolve rule, and the shape #3282 measured as "fired at 99% and resolved + to 23% within two minutes": an open incident that dips under the bar for one sample is still + the same incident, so no resolve/re-fire pair is produced. */ var h = new Harness(); h.Settings.CpuEnabled = true; var engine = h.Build(); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 90, suppressed: true)); + var at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 99, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase); + at = await DriveCpuAsync(engine, sqlCpu: 20, totalCpu: 23, samples: 1, from: at); + await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 99, samples: 1, from: at); + + Assert.Single(h.Deliverer.Outcomes); + Assert.Empty(h.Resolutions); + } + + [Fact] + public async Task Cpu_MissingValueFreezesTheGate_AndNeverAnnouncesARecovery() + { + /* A CPU sample that stops arriving is not a recovery. The pre-#3282 code fell through to its + resolve arm on a null value and sent "SRV-A: Total CPU back to %" — a recovery message with + no number in it, about a measurement nobody took. The streak is frozen instead, so the sample + that eventually arrives continues where the last real one left off. */ + var h = new Harness(); + h.Settings.CpuEnabled = true; + var engine = h.Build(); + + var at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 90, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase); + Assert.Single(h.Deliverer.Outcomes); + + at = await DriveCpuAsync(engine, sqlCpu: null, totalCpu: null, samples: 5, from: at); + Assert.Empty(h.Resolutions); + + /* And the frozen streak is the BREACH streak: one real clear sample still is not enough. */ + at = await DriveCpuAsync(engine, sqlCpu: 20, totalCpu: 40, samples: 1, from: at); + Assert.Empty(h.Resolutions); + + await DriveCpuAsync(engine, sqlCpu: 20, totalCpu: 40, samples: 1, from: at); + Assert.Single(h.Resolutions); + } + + [Fact] + public async Task Cpu_NoSampleInstant_CountsEverySweep() + { + /* The documented DEGRADATION for a host that supplies no sample instant (see + AlertServerSnapshot.CpuSampleTimeUtc). Persistence is then per-sweep rather than per-sample — + weaker, but the alert still fires, because silence is the one failure a monitoring product + cannot tell apart from health. Pinned so the fallback is a decision rather than an accident. */ + var h = new Harness(); + h.Settings.CpuEnabled = true; + var engine = h.Build(); + + for (var i = 0; i < AlertEngine.CpuBreachSamples; i++) + { + await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 95, noCpuSampleTime: true)); + } + + Assert.Single(h.Deliverer.Outcomes); + } + + [Fact] + public async Task Cpu_PersistsTheStreakAndResumesAcrossARestart() + { + /* Erik's restart requirement, first half: a partly-built streak must not be lost in a way that + makes a sustained event never fire. It is PERSISTED, so a new engine over the same store + resumes and the very next sample completes the streak — rather than restarting the count, which + would delay a real saturation event by CpuBreachSamples samples on every service restart. */ + var h = new Harness(); + h.Settings.CpuEnabled = true; + + var at = await DriveCpuAsync(h.Build(), sqlCpu: 70, totalCpu: 95, samples: AlertEngine.CpuBreachSamples - 1, from: Harness.SampleBase); Assert.Empty(h.Deliverer.Outcomes); - /* Un-suppressed one second later: fires immediately — no cooldown was stamped. */ - h.Now = h.Now.AddSeconds(1); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 90)); + var persisted = Assert.Single(h.StateStore.Persistence); + Assert.Equal(AlertEngine.CpuPersistenceMetric, persisted.Key.Metric); + Assert.Equal(AlertEngine.CpuBreachSamples - 1, persisted.Value.State.ConsecutiveBreaches); + Assert.False(persisted.Value.State.Firing); + + /* A brand-new engine over the same state store IS the restart. */ + await DriveCpuAsync(h.Build(), sqlCpu: 70, totalCpu: 95, samples: 1, from: at); Assert.Single(h.Deliverer.Outcomes); } + [Fact] + public async Task Cpu_ARestartDoesNotReAnnounceAnAlreadyOpenIncident() + { + /* Erik's restart requirement, second half, and the reason the Firing bit had to leave memory: the + pre-#3282 flag was in-memory only, so the first post-restart sweep over a standing condition + delivered the same incident again. The persisted bit means the restarted engine knows the + incident is open, and the only thing that can produce another message is the ordinary cooldown + reminder — which is why the clock is left alone here. */ + var h = new Harness(); + h.Settings.CpuEnabled = true; + + var at = await DriveCpuAsync(h.Build(), sqlCpu: 70, totalCpu: 95, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase); + Assert.Single(h.Deliverer.Outcomes); + Assert.True(Assert.Single(h.StateStore.Persistence).Value.State.Firing); + + var restarted = h.Build(); + at = await DriveCpuAsync(restarted, sqlCpu: 70, totalCpu: 95, samples: 1, from: at); + Assert.Single(h.Deliverer.Outcomes); + + /* And it resolves normally rather than being orphaned. */ + at = await DriveCpuAsync(restarted, sqlCpu: 20, totalCpu: 30, samples: AlertEngine.CpuClearSamples, from: at); + Assert.Single(h.Resolutions); + } + + [Fact] + public async Task Cpu_TheGateAdvancesUnderSuppression_SoOneUnsuppressedSampleDelivers() + { + /* Suppression is evaluate-but-don't-deliver, and that has to remain true of the GATE and not just + of the send: a suppressed streak must count, so that un-acknowledging a server reports the + condition it is actually in rather than starting a fresh three-sample wait. + + This pin exists because its Lite twin cannot be run here. Lite.Tests is net10.0-windows and its + harness is the WPF app, so LiteAlertForwardingTests.SuppressedSweep_... is CI's to execute — + and it is the exact claim that test now depends on after #3282. The engine is shared, so + asserting it here makes the Lite edit a verified claim rather than a plausible one. */ + var h = new Harness(); + h.Settings.CpuEnabled = true; + var engine = h.Build(); + + var at = await DriveCpuAsync(engine, sqlCpu: 95, totalCpu: 99, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase, suppressed: true); + Assert.Empty(h.Deliverer.Outcomes); + + /* One UNsuppressed sample: the incident is already open from the suppressed streak, so this is the + standing-condition delivery rather than a rising edge that has to be earned again. */ + await DriveCpuAsync(engine, sqlCpu: 95, totalCpu: 99, samples: 1, from: at); + Assert.Single(h.Deliverer.Outcomes); + } + + [Fact] + public async Task Cpu_APersistenceSaveFailure_StillFires_AndIsNotCountedAsASwallowedRead() + { + /* Two claims, and the second is the one that is easy to get wrong — I did. + + The gate decides each observation from its IN-MEMORY record, so a store that cannot be written + costs the streak across a restart and never an alert: the incident still fires on the third + sample. + + And the failure is NOT recorded on #3013's counter. That counter is about READS the alert pass + performs and swallows, against a denominator of alert passes, and an operator reads a non-zero + value as the pass going blind on a condition. A write in its numerator says that about a + condition that was in fact evaluated correctly — only the memory of it was lost. Same call + SaveOccurrencesAsync already makes for the same reason, and it is a claim no compiler or + text-assertion can carry. */ + var counter = new AlertReadFailureCounter(() => new DateTime(2026, 9, 5, 8, 0, 0, DateTimeKind.Utc)); + var store = new ThrowingPersistenceSaveStore(); + var h = new Harness { ReadFailures = counter }; + h.Settings.CpuEnabled = true; + + var engine = new AlertEngine( + h.Settings, h.Adapter, store, h.Deliverer, _ => false, + resolutionCallback: (r, _) => { h.Resolutions.Add(r); return Task.CompletedTask; }, + utcNow: () => h.Now, readFailures: counter); + + await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 95, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase); + + Assert.True(store.SaveAttempts > 0, "the engine must have tried to persist, or this pin proves nothing"); + Assert.Single(h.Deliverer.Outcomes); + Assert.Equal(0, counter.ReadFor(Key).ServerReadFailures); + Assert.Equal(0, counter.ReadFor(Key).InstanceReadFailures); + + /* And it resolves normally too — a broken save must not strand an open incident. */ + await DriveCpuAsync(engine, sqlCpu: 20, totalCpu: 30, samples: AlertEngine.CpuClearSamples, + from: Harness.SampleBase.AddMinutes(AlertEngine.CpuBreachSamples)); + Assert.Single(h.Resolutions); + } + + [Fact] + public void CpuGateDefaults_AreDerivedFromTheSampleCadence() + { + /* The numbers are a real decision, so they are pinned rather than left to drift silently. Three + breaching samples at the ~60-second SCHEDULER_MONITOR cadence is about three minutes, which is + above every excursion measured on the fleet for #3282 (42-147 seconds, median 87 s, worst two + consecutive one-minute samples over the bar). Clearing is deliberately FASTER than firing: a + late resolve is a stale open incident, an early one is the resolve/fire pair this issue exists + to remove. */ + Assert.Equal(3, AlertEngine.CpuBreachSamples); + Assert.Equal(2, AlertEngine.CpuClearSamples); + Assert.True(AlertEngine.CpuClearSamples < AlertEngine.CpuBreachSamples); + Assert.True(AlertEngine.CpuClearSamples > 1); + + /* The persisted subject is the metric an operator already knows, not a second spelling of it. */ + Assert.Equal("High CPU", AlertEngine.CpuPersistenceMetric); + } + /* ---------------- mute ---------------- */ [Fact] @@ -467,13 +819,13 @@ public async Task MutedAlert_IsDeliveredFlaggedMuted_AndStampsTheCooldown() h.Muted = true; var engine = h.Build(); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 90)); + var at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 90, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase); Assert.True(Assert.Single(h.Deliverer.Outcomes).Muted); /* Unmuting inside the cooldown does not re-fire — the muted fire stamped it (:76). */ h.Muted = false; h.Now = h.Now.AddMinutes(1); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 90)); + await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 90, samples: 1, from: at); Assert.Single(h.Deliverer.Outcomes); } @@ -1669,8 +2021,13 @@ public async Task StatePerServer_IsIndependent() h.Settings.CpuEnabled = true; var engine = h.Build(); - await engine.EvaluateServerAsync(new AlertServerSnapshot("101", "SRV-A", true, 70, 90, false, false)); - await engine.EvaluateServerAsync(new AlertServerSnapshot("202", "SRV-B", true, 70, 90, false, false)); + /* #3282: CpuBreachSamples distinct samples per server, since High CPU no longer fires on one. */ + for (var i = 1; i <= AlertEngine.CpuBreachSamples; i++) + { + var at = Harness.SampleBase.AddMinutes(i); + await engine.EvaluateServerAsync(new AlertServerSnapshot("101", "SRV-A", true, 70, 90, false, false, at)); + await engine.EvaluateServerAsync(new AlertServerSnapshot("202", "SRV-B", true, 70, 90, false, false, at)); + } Assert.Equal(2, h.Deliverer.Outcomes.Count); Assert.Equal(new[] { "101", "202" }, h.Deliverer.Outcomes.Select(o => o.ServerKey).ToArray()); diff --git a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs index a900a12f1..c599ba9ac 100644 --- a/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs +++ b/Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs @@ -359,7 +359,7 @@ real failure rather than a matcher that never matches anything. */ /// private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScopes = { - (Path.Combine("PerformanceMonitor.Alerting", "AlertEngine.cs"), 13, 4), + (Path.Combine("PerformanceMonitor.Alerting", "AlertEngine.cs"), 13, 5), (Path.Combine("Darling", "PerformanceMonitor.Darling.Service", "DarlingSelfAlertEvaluator.cs"), 5, 8), }; @@ -416,6 +416,7 @@ private static readonly (string Path, int Counted, int Exempt)[] s_wholeFileScop { ["Could not load incident occurrences"] = "bookkeeping about an alert, not the condition read it is judged on", ["Could not persist incident occurrences"] = "a write", + ["Could not persist the CPU persistence gate"] = "a write (#3282); the gate has already decided the observation from its in-memory record, so a dropped save costs the streak across a restart and never an alert - the seeding LOAD beside it is the read, and it is counted", ["Alert resolution callback failed"] = "the delivery path", ["Connection-change self-alert delivery failed"] = "the delivery path", ["Store disk-pressure self-alert failed"] = "handed its evidence as parameters; the read is counted in DarlingWorker", @@ -519,7 +520,7 @@ an off-by-one on a total. */ /* The whole-tree totals, so a site MOVED between the scoped regions still has to be re-counted by a person rather than netting out silently. */ Assert.Equal(CountedSites, totalCounted); - Assert.Equal(19, totalExempt); + Assert.Equal(20, 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 diff --git a/Darling/Darling.Tests/BuiltinAlertPersistenceRungTests.cs b/Darling/Darling.Tests/BuiltinAlertPersistenceRungTests.cs new file mode 100644 index 000000000..e4e37380f --- /dev/null +++ b/Darling/Darling.Tests/BuiltinAlertPersistenceRungTests.cs @@ -0,0 +1,382 @@ +/* + * 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.Linq; +using System.Reflection; +using PerformanceMonitor.Alerting; +using PerformanceMonitor.Common; +using PerformanceMonitor.Darling.Storage; +using PerformanceMonitor.Darling.Viewer; +using Xunit; + +namespace Darling.Tests; + +/// +/// V118 / #3282: the built-in alert catalog's persistence-gate state (config.alert_persistence_state). +/// This carries the "I am the top rung" claims that moved off +/// (V117) when this rung landed — a fully-migrated store must map to EXACTLY this version, or the viewer's +/// connect-time gate refuses a store that is actually current. +/// +public sealed class BuiltinAlertPersistenceRungTests +{ + private const int RungVersion = 118; + private const int PreviousVersion = 117; + + /// This rung's sentinel ordinal in the viewer probe — the newest, so the last argument. + private const int ProbeOrdinal = 93; + + [Fact] + public void TheRungIsRegisteredAtTheTopOfADenseLadder() + { + var versions = PgMigrations.Scripts.Select(s => s.Version).ToList(); + + Assert.Equal( + "builtin-alert-persistence", + PgMigrations.Scripts.Single(s => s.Version == RungVersion).Name); + + Assert.Equal(StorageVersion.SchemaVersion, PgMigrations.Scripts[^1].Version); + Assert.Equal(StorageVersion.SchemaVersion, versions.Max()); + Assert.Equal(RungVersion, StorageVersion.SchemaVersion); + + Assert.Equal(versions.Distinct().OrderBy(v => v), versions); + } + + [Fact] + public void TheRungCreatesTheStateTable_SchemaQualified_WithNoBumpTrigger() + { + var rung = PgMigrations.Scripts.Single(s => s.Version == RungVersion).Sql; + + /* Schema-qualified for the reason every config rung is: the migrate session's search_path puts + collect first, so a bare name would resolve to the wrong schema (and the wrong ACL). */ + Assert.Contains("CREATE TABLE IF NOT EXISTS config.alert_persistence_state", rung, StringComparison.Ordinal); + + /* The subject is (server_id, metric_name) — the key the engine's other state already uses, and + the reason this could not be a row in custom_alert_state (keyed on a rule that does not exist + for a built-in alert). */ + Assert.Contains("PRIMARY KEY (server_id, metric_name)", rung, StringComparison.Ordinal); + + /* The gate's three state fields plus its observation identity. last_observed_sample_at is the one + that makes the counters mean SAMPLES rather than sweeps, so it is asserted explicitly: without + it "three consecutive breaches" is satisfied inside ninety seconds by one re-read row. */ + foreach (var column in new[] + { + "consecutive_breaches", "consecutive_clears", "firing", "last_observed_sample_at", + }) + { + Assert.Contains(column, rung, StringComparison.Ordinal); + } + + /* Naive UTC, the store convention — a timestamptz here would be zone-shifted against every other + timestamp it is compared with. */ + Assert.Contains("timestamp", rung, StringComparison.Ordinal); + Assert.DoesNotContain("timestamptz", rung, StringComparison.Ordinal); + + /* No reload beacon: this is evaluator state written on every new sample, and a bump would force a + fleet-wide ReloadFromStoreAsync about once a minute per server. */ + Assert.DoesNotContain("config_bump_version", rung, StringComparison.Ordinal); + } + + [Fact] + public void TheProbeMapsAFullyMigratedStoreToThisTopRung() + { + Assert.Contains( + "table_name = 'alert_persistence_state'", + ViewerDataService.StoreSchemaProbeSql, StringComparison.Ordinal); + + var viewer = RepoFile.ReadRepoFile("Darling", "PerformanceMonitor.Darling.Viewer", "ViewerDataService.cs"); + Assert.Contains($"reader.GetBoolean({ProbeOrdinal})", viewer, StringComparison.Ordinal); + Assert.Contains("hasBuiltinAlertPersistence", viewer, StringComparison.Ordinal); + + Assert.Equal(StorageVersion.SchemaVersion, ViewerDataService.RequiredStoreSchemaVersion); + + var method = typeof(ViewerDataService) + .GetMethod("MapProbedSchemaVersion", BindingFlags.NonPublic | BindingFlags.Static)!; + var arity = method.GetParameters().Length; + + /* The top rung's sentinel IS the last argument. */ + Assert.Equal(ProbeOrdinal, arity - 1); + + /* Every sentinel true = a fully-migrated store, which must map to exactly this version. Built by + reflection so the arity tracks the signature. */ + var all = Enumerable.Repeat((object)true, arity).ToArray(); + Assert.Equal(StorageVersion.SchemaVersion, (int)method.Invoke(null, all)!); + + /* One rung behind: every sentinel EXCEPT this one reports 116 (the previous top rung). */ + var behind = Enumerable.Repeat((object)true, arity).ToArray(); + behind[ProbeOrdinal] = false; + Assert.Equal(PreviousVersion, (int)method.Invoke(null, behind)!); + } + + [Fact] + public void TheStoreReadAndWriteNameTheRungsColumns() + { + /* The rung is inert unless the store that reads and writes it names the same columns. Asserted + against the shipped source rather than a retyped copy, for the reason the PostgreSQL work + learned the hard way: a proven query and a working feature are different claims, and every + blocking defect in that slice was a call site rather than the SQL. + + Not a live-store test, deliberately: Darling PostgreSQL tests cover the migration applying, and + what can go wrong HERE is a column renamed on one side of the seam, which is a text fact. */ + var store = RepoFile.ReadRepoFile("Darling", "PerformanceMonitor.Darling.Service", "PgAlertStateStore.cs"); + + Assert.Contains("config.alert_persistence_state", store, StringComparison.Ordinal); + Assert.Contains("ON CONFLICT (server_id, metric_name) DO UPDATE SET", store, StringComparison.Ordinal); + + foreach (var column in new[] + { + "consecutive_breaches", "consecutive_clears", "firing", "last_observed_sample_at", + }) + { + Assert.Contains(column, store, StringComparison.Ordinal); + } + } + + /// + /// The PostgreSQL High CPU evaluator is wired to the SAME gate, with the SAME constants, and reads a + /// BATCH of samples rather than the latest one. + /// + /// The batch is the load-bearing half and it is engine-specific. On SQL Server the 30-second + /// sweep is faster than the ~60-second sample, so "latest row, counted once when its instant moves" + /// loses nothing. Performance Insights is sampled at 60 seconds and pg_cpu_utilization is a + /// five-minute collector, so five points land together: counting only the newest would discard four in + /// five and make three consecutive breaches take three BATCHES, about fifteen minutes. That is a + /// saturation event reported long after it mattered, and it is exactly the failure the issue's own + /// brief warned a too-high N would cause — reached here by an invisible route rather than by choosing + /// a number. + /// + /// Source assertions rather than a driven test because this evaluator lives inside + /// DarlingWorker behind an AWS client and a live store; the gate's arithmetic is covered by + /// AlertPersistenceGateTests and its SQL Server wiring by AlertEngineTests, so what is + /// left to go wrong here is a call site, which is a text fact. Sliced to this method so an assertion + /// cannot be satisfied by the SQL Server CPU alert's neighbour text. + /// + [Fact] + public void ThePostgresCpuAlertUsesTheSharedGate_OverABatchOfSamples() + { + var body = EvaluatePgCpuBody(); + + Assert.Contains("AlertPersistenceGate.Evaluate", body, StringComparison.Ordinal); + Assert.Contains("AlertEngine.CpuBreachSamples", body, StringComparison.Ordinal); + Assert.Contains("AlertEngine.CpuClearSamples", body, StringComparison.Ordinal); + + /* No second set of numbers: the whole point of #3282 is one mechanism, so a local constant or a + bare literal here would be the drift the issue predicted. */ + Assert.DoesNotContain("breachSamples: 3", body, StringComparison.Ordinal); + Assert.DoesNotContain("clearSamples: 2", body, StringComparison.Ordinal); + + /* TWO READS, TWO ROLES, and the roles are what is pinned rather than which names appear. + The first spelling of this pin asserted GetLatestAsync was ABSENT, which encoded "the batch + replaced it" — and that was the wrong property. The gate counts new samples, so it reads the + batch. The standing-condition REMINDER asks whether the condition is still there, which is a + different question and has to be answerable on a sweep that brought no new sample: the sweep is + 30 seconds and the collector is five minutes, so most sweeps bring none. Asserting a name's + absence made a parity break look correct — the reminder's cadence silently capped at the + collector interval instead of the configured cooldown (review catch). */ + Assert.Contains("GetSamplesSinceAsync", body, StringComparison.Ordinal); + + /* The gate's observation is the batch's sample, never the fallback reading. */ + var gate = body.IndexOf("AlertPersistenceGate.Evaluate", StringComparison.Ordinal); + var fallback = body.IndexOf("GetLatestAsync", StringComparison.Ordinal); + Assert.True(fallback > gate, "the latest-reading fallback must sit AFTER the gate loop, so it cannot advance the gate"); + + /* And it is reached only when the batch brought nothing AND an incident is open — it exists for + the reminder, not as a second way to observe. */ + Assert.Contains("if (!lastCapacityPercent.HasValue && record.State.Firing)", body, StringComparison.Ordinal); + + /* breaching is computed AFTER the fallback, or the fallback informs nothing. */ + var breaching = body.IndexOf("bool breaching = lastCapacityPercent.HasValue", StringComparison.Ordinal); + Assert.True(breaching > fallback, "breaching must be computed after the fallback that populates it"); + + /* It persists through the same seam under the same subject key, so a restart does not re-announce + an open incident and the two engines' rows are one shape. */ + Assert.Contains("stateStore.LoadAlertPersistenceAsync", body, StringComparison.Ordinal); + Assert.Contains("stateStore.SaveAlertPersistenceAsync", body, StringComparison.Ordinal); + Assert.Contains("AlertEngine.CpuPersistenceMetric", body, StringComparison.Ordinal); + + /* The active-flag it replaced is gone from the whole file, not just from this method — a leftover + would be a second source of truth for "is an incident open". */ + var worker = RepoFile.ReadRepoFileLf("Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"); + Assert.DoesNotContain("_activePgCpuAlert", worker, StringComparison.Ordinal); + } + + /// + /// A restart must not re-announce an already-open incident on EITHER engine. The persisted Firing bit + /// stops a second rising edge, but the cooldown dictionaries are in-memory on both sides, so the + /// seeding has to stamp the clock too — and doing that on only one of the two engines is the + /// shared-seam half-fix this whole issue is about, in miniature. + /// + [Fact] + public void ARestartStampsTheCooldownOnBothEngines() + { + var pg = EvaluatePgCpuBody(); + Assert.Contains("if (seeded.Value.State.Firing)", pg, StringComparison.Ordinal); + Assert.Contains("_lastPgCpuAlert[key] = now;", pg, StringComparison.Ordinal); + + var engine = RepoFile.ReadRepoFileLf("PerformanceMonitor.Alerting", "AlertEngine.cs"); + Assert.Contains("if (cpuPersistence.Value.State.Firing)", engine, StringComparison.Ordinal); + Assert.Contains("_lastCpuAlert[key] = _utcNow();", engine, StringComparison.Ordinal); + + /* And neither engine bypasses the cooldown for the rising edge, so one threshold means one thing + across them — the parity #2719 chose the shared metric names for. */ + Assert.DoesNotContain("cooldownElapsed = fired", pg, StringComparison.Ordinal); + } + + /// + /// AT MOST ONE EDGE PER PASS, which is the invariant that makes a whole defect class unreachable + /// rather than guarded. + /// + /// The batch read introduced it. Accumulating a batch's edges into flags and deciding at the end + /// compresses a SEQUENCE into a summary, and that lost an edge twice: first a fire and a resolve for + /// the same incident flattening into "both happened", then — the review catch — a pre-existing + /// incident's resolve being overwritten by a later, unrelated fire in the same batch, so nobody heard + /// that the incident they had open was over. Confirmed reachable at seven samples + /// (CpuClearSamples + CpuBreachSamples + CpuClearSamples), well inside the batch limit and the + /// freshness window, i.e. one restart or one collector gap. + /// + /// Stopping at the first edge makes both instances impossible, and makes this pass the same + /// shape as AlertEngine's SQL Server twin: one observation, one edge, the fire and resolve arms + /// mutually exclusive by construction. So what is pinned is the break and the absence of the + /// accumulators — a reintroduced flag is the return of the category. + /// + [Fact] + public void ThePassStopsAtTheFirstEdge_SoNoEdgeSequenceIsEverFlattened() + { + var body = EvaluatePgCpuBody(); + + /* The edge is taken and the loop stops. */ + Assert.Contains("if (evaluation.Outcome != PersistenceOutcome.None)", body, StringComparison.Ordinal); + var edge = body.IndexOf("if (evaluation.Outcome != PersistenceOutcome.None)", StringComparison.Ordinal); + var close = body.IndexOf("\n }", edge, StringComparison.Ordinal); + Assert.True(close > edge, "the edge arm's end was not found, so this pin would read nothing"); + Assert.Contains("break;", body[edge..close], StringComparison.Ordinal); + + /* And the accumulators are GONE. Either one coming back is the category returning, because both + lost edges were produced by exactly this shape. */ + Assert.DoesNotContain("bool fired = false", body, StringComparison.Ordinal); + Assert.DoesNotContain("bool resolved = false", body, StringComparison.Ordinal); + Assert.DoesNotContain("fired && resolved", body, StringComparison.Ordinal); + + /* The two arms read the ONE edge, so they cannot both run. */ + Assert.Contains("else if (outcome == PersistenceOutcome.Resolve)", body, StringComparison.Ordinal); + + /* One more route to the same loss, closed by the same break: an edge taken but not persisted would + be replayed or skipped depending on which side of the save it fell. The save is after the loop + and before both arms, so the state at the edge is committed whatever the arms do. */ + var save = body.IndexOf("stateStore.SaveAlertPersistenceAsync", StringComparison.Ordinal); + var fire = body.IndexOf("if (record.State.Firing && breaching)", StringComparison.Ordinal); + Assert.True(save > edge, "the state save must come after the loop that takes the edge"); + Assert.True(fire > save, "the state save must come before the delivery arms"); + } + + /// + /// A missing capacity reading must FREEZE the gate rather than clear it. #3281 established that the + /// alert never FIRES on an absent capacity sample (a fallback to percent-of-allocated silently arms the + /// threshold against the wrong denominator); the other half is that it must not RESOLVE on one either, + /// because that announces a recovery nobody measured. The pre-#3282 code treated absent capacity as + /// "not exceeded", which is the resolve arm. + /// + [Fact] + public void AnAbsentCapacityReadingIsNotAClear() + { + var body = EvaluatePgCpuBody(); + + Assert.Contains("if (!capacityPercent.HasValue)", body, StringComparison.Ordinal); + + /* `continue` is the freeze: the sample is neither counted nor marked observed, so the streak holds + and a later reading picks up where the last real one left off. */ + var absent = body.IndexOf("if (!capacityPercent.HasValue)", StringComparison.Ordinal); + var next = body.IndexOf("AlertPersistenceGate.Evaluate", absent, StringComparison.Ordinal); + Assert.True(next > absent, "the no-capacity arm must sit before the gate call it skips"); + Assert.Contains("continue;", body[absent..next], StringComparison.Ordinal); + } + + /// The EvaluatePgCpuAsync body, sliced from its own declaration to the next member at + /// the same indent — sliced rather than searched whole-file because the SQL Server CPU alert's text + /// lives in sibling files and a pin that reads a neighbour is not a pin. The integrity assertions are + /// the same shape PgCpuCapacityHeadroomTests uses on the same method. + private static string EvaluatePgCpuBody() + { + var source = RepoFile.ReadRepoFileLf( + "Darling", "PerformanceMonitor.Darling.Service", "DarlingWorker.cs"); + + const string start = "private async Task EvaluatePgCpuAsync("; + var from = source.IndexOf(start, StringComparison.Ordinal); + Assert.True(from >= 0, "EvaluatePgCpuAsync was not found, so this pin would read nothing"); + + var to = source.IndexOf("\n private ", from + start.Length, StringComparison.Ordinal); + Assert.True(to > from, "the method's end was not found, so this pin would read the rest of the file"); + + var body = source[from..to]; + + /* The slice has to be a method, not a fragment: an off-by-one on either bound silently shrinks it + and every Assert.DoesNotContain above starts passing for the wrong reason. */ + Assert.Contains("await DarlingPgCpuUtilizationReader.GetSamplesSinceAsync", body, StringComparison.Ordinal); + Assert.True(body.Length > 1500, $"the sliced body is only {body.Length} chars, which cannot be this method"); + + return body; + } + + /// + /// The README's alert catalog states the sample count, so the number is pinned to the constant rather + /// than left as a prose copy of it. A doc that says "3 samples" while the code says something else is + /// a stale count with a numeral welded on, and nothing else in the build would notice — the whole + /// point of the catalog row is that it is what a user reads before tuning the threshold. + /// + [Fact] + public void TheReadmeAlertCatalogStatesTheSampleCountItActuallyUses() + { + var readme = RepoFile.ReadRepoFileLf("README.md"); + + var row = readme + .Split('\n') + .Single(l => l.StartsWith("| **High CPU**", StringComparison.Ordinal)); + + Assert.Contains($"held for {AlertEngine.CpuBreachSamples} samples", row, StringComparison.Ordinal); + Assert.Contains($"{AlertEngine.CpuBreachSamples} consecutive collected samples", row, StringComparison.Ordinal); + Assert.Contains($"{AlertEngine.CpuClearSamples} consecutive samples below it", row, StringComparison.Ordinal); + + /* NUMERALS, not spelled-out words, and this is the reason rather than a style preference: the row + first read "three consecutive collected samples", which this pin could not match against the + constant. A doc count that cannot be compared to the thing it describes is a count nobody can + check. */ + + /* And it must not still claim the pre-#3282 rule, which is the sentence a reader would act on. */ + Assert.DoesNotContain("Fires when total CPU (SQL + other) exceeds the threshold |", row, StringComparison.Ordinal); + } + + /// + /// The batch read is bounded and floors its lower bound at the freshness window, so a subject with no + /// memory (or a collector that was away) considers only readings recent enough to describe "right now" + /// — the same bound the single-reading path applies, so the gate and the card cannot disagree about + /// what counts as current. + /// + [Fact] + public void TheBatchReadIsBoundedAndSharesTheFreshnessWindow() + { + Assert.Contains("sample_time > $2", DarlingPgCpuUtilizationReader.SamplesSinceSql, StringComparison.Ordinal); + Assert.Contains("LIMIT $3", DarlingPgCpuUtilizationReader.SamplesSinceSql, StringComparison.Ordinal); + + /* OLDEST FIRST. Descending order would feed the gate backwards, which produces a plausible count + from an impossible sequence: a recovery followed by a breach would read as a breach followed by + a recovery, and the streak would be built out of samples in the wrong direction. */ + Assert.Contains("ORDER BY sample_time\n", DarlingPgCpuUtilizationReader.SamplesSinceSql.Replace("\r\n", "\n"), StringComparison.Ordinal); + Assert.DoesNotContain("ORDER BY sample_time DESC", DarlingPgCpuUtilizationReader.SamplesSinceSql, StringComparison.Ordinal); + + /* A null capacity is stored as NULL rather than 0, and the gate must never see a fabricated + reading — so the read filters on cpu_percent only, like its sibling, and leaves the capacity + nullable for the evaluator's own named no-capacity state. */ + Assert.Contains("cpu_percent IS NOT NULL", DarlingPgCpuUtilizationReader.SamplesSinceSql, StringComparison.Ordinal); + Assert.DoesNotContain("acu_utilization_percent IS NOT NULL", DarlingPgCpuUtilizationReader.SamplesSinceSql, StringComparison.Ordinal); + + /* Bounded above by a constant that cannot change the outcome — the gate's counters saturate at + their thresholds — so this only stops one pass reading an unbounded list. */ + Assert.True(DarlingPgCpuUtilizationReader.GateBatchLimit >= AlertEngine.CpuBreachSamples + AlertEngine.CpuClearSamples); + Assert.True(DarlingPgCpuUtilizationReader.GateBatchLimit <= 128); + } +} diff --git a/Darling/Darling.Tests/CustomAlertCoreMigrationTests.cs b/Darling/Darling.Tests/CustomAlertCoreMigrationTests.cs index 2e9c416cd..0ec6a2a1f 100644 --- a/Darling/Darling.Tests/CustomAlertCoreMigrationTests.cs +++ b/Darling/Darling.Tests/CustomAlertCoreMigrationTests.cs @@ -17,8 +17,10 @@ namespace Darling.Tests; /// /// V116 / #3285: the custom-alert core rung (config.custom_alert_rules + config.custom_alert_state). The -/// "I am the top rung" claims live on (V117); this rung is strictly -/// below the top, and its probe arm has to keep reporting 116 for a store migrated exactly this far. +/// "I am the top rung" claims live on whichever rung is currently top — +/// took them from here at V117 and holds them at V118. This +/// rung is strictly below the top either way, and its probe arm has to keep reporting 116 for a store +/// migrated exactly this far. /// public sealed class CustomAlertCoreMigrationTests { diff --git a/Darling/Darling.Tests/DarlingAlertingTests.cs b/Darling/Darling.Tests/DarlingAlertingTests.cs index ffc870152..342b349be 100644 --- a/Darling/Darling.Tests/DarlingAlertingTests.cs +++ b/Darling/Darling.Tests/DarlingAlertingTests.cs @@ -265,7 +265,9 @@ AlertEngine BuildEngine(RecordingDeliverer deliverer, MuteRuleService muteRuleSe var snapshot = new AlertServerSnapshot( TestServerKey, TestServerName, IsOnline: true, SqlCpuPercent: null, TotalCpuPercent: null, - IsAzureSqlDb: false, Suppressed: false); + IsAzureSqlDb: false, Suppressed: false, + /* No CPU value at all here, so the #3282 gate never observes anything on this path. */ + CpuSampleTimeUtc: null); /* --- first sweep: the deadlock and the poison wait fire, unmuted --- */ var (deliverer, engine) = await BuildStackAsync(); diff --git a/Darling/Darling.Tests/DarlingSelfAlertTests.cs b/Darling/Darling.Tests/DarlingSelfAlertTests.cs index e16805686..094e9fbc2 100644 --- a/Darling/Darling.Tests/DarlingSelfAlertTests.cs +++ b/Darling/Darling.Tests/DarlingSelfAlertTests.cs @@ -2129,14 +2129,26 @@ await history.RecordAlertAsync(DarlingSelfAlertEvaluator.BuildResolutionRecord(r logger: null, utcNow: () => now); - /* Fire: total CPU 90 >= 80. */ - await engine.EvaluateServerAsync(new AlertServerSnapshot(Key, Name, IsOnline: true, 90, 90, false, false), Ct); + /* Fire: total CPU 90 >= 80, held for AlertEngine.CpuBreachSamples distinct samples (#3282 — one + sample over the bar is no longer an incident). */ + var sampleAt = new DateTime(2026, 7, 1, 0, 0, 0, DateTimeKind.Utc); + for (var i = 0; i < AlertEngine.CpuBreachSamples; i++) + { + sampleAt = sampleAt.AddMinutes(1); + await engine.EvaluateServerAsync(new AlertServerSnapshot(Key, Name, IsOnline: true, 90, 90, false, false, sampleAt), Ct); + } + Assert.Single(deliverer.Outcomes); Assert.Empty(history.Records); /* no resolution yet */ - /* Clear: CPU back below threshold => the engine emits a resolution => a history row is written. */ + /* Clear: CPU back below threshold for AlertEngine.CpuClearSamples distinct samples => the engine + emits a resolution => a history row is written. */ now = now.AddMinutes(1); - await engine.EvaluateServerAsync(new AlertServerSnapshot(Key, Name, IsOnline: true, 10, 10, false, false), Ct); + for (var i = 0; i < AlertEngine.CpuClearSamples; i++) + { + sampleAt = sampleAt.AddMinutes(1); + await engine.EvaluateServerAsync(new AlertServerSnapshot(Key, Name, IsOnline: true, 10, 10, false, false, sampleAt), Ct); + } var resolved = Assert.Single(history.Records); Assert.Equal("CPU Resolved", resolved.MetricName); Assert.Equal(AlertDelivery.ChannelNotApplicable, resolved.NotificationType); @@ -2194,6 +2206,21 @@ public Task> LoadIncidentOc new Dictionary(StringComparer.Ordinal)); public Task SaveIncidentOccurrencesAsync(string serverKey, string metricName, IReadOnlyDictionary states) => Task.CompletedTask; + + /* #3282: real, for the same reason the other two fakes are. These tests drive the SELF-alert paths + and never the CPU check, so nothing here reads it back — but a stub that answered "no memory" to + a load and swallowed every save is indistinguishable from the seam being wired wrong, and this + class already has one stub-shaped no-op above that had to be justified in a comment. */ + public Dictionary<(string Key, string Metric), AlertPersistenceRecord> Persistence { get; } = new(); + + public Task LoadAlertPersistenceAsync(string serverKey, string metricName) => + Task.FromResult(Persistence.TryGetValue((serverKey, metricName), out var r) ? (AlertPersistenceRecord?)r : null); + + public Task SaveAlertPersistenceAsync(string serverKey, string metricName, AlertPersistenceRecord record) + { + Persistence[(serverKey, metricName)] = record; + return Task.CompletedTask; + } } /* ---------------- live collection_log reads (gated on DARLING_TEST_PG) ---------------- */ diff --git a/Darling/Darling.Tests/MuteRuleReloadBeaconTests.cs b/Darling/Darling.Tests/MuteRuleReloadBeaconTests.cs index 9b2d7b00f..6146d3d04 100644 --- a/Darling/Darling.Tests/MuteRuleReloadBeaconTests.cs +++ b/Darling/Darling.Tests/MuteRuleReloadBeaconTests.cs @@ -28,9 +28,11 @@ namespace Darling.Tests; /// confirm it — the rule list, the tool's own success reply — reads the TABLE, so they all agree the mute is /// in place while matching alerts keep being delivered. /// -/// This class also carries the "I am the top rung" claims, handed over from -/// (V116) when this rung landed: a fully-migrated store must map -/// to EXACTLY this version, or the viewer's connect-time gate refuses a store that is current. +/// The "I am the top rung" claims have moved on to +/// (V118), the way this rung took them from (V116) — the +/// documented hand-over when a later rung merges. What stays here is this rung's own identity and its probe +/// arm, which must keep mapping a store migrated to EXACTLY 117 to 117 rather than letting it fall +/// through. /// public sealed class MuteRuleReloadBeaconTests { @@ -51,7 +53,10 @@ public void TheRungIsRegisteredAtTheTopOfADenseLadder() Assert.Equal(StorageVersion.SchemaVersion, PgMigrations.Scripts[^1].Version); Assert.Equal(StorageVersion.SchemaVersion, versions.Max()); - Assert.Equal(RungVersion, StorageVersion.SchemaVersion); + + /* V118 (#3282) is the top rung now, so the "== SchemaVersion" claim lives there. Strictly LESS + rather than <=, so this cannot silently become the top-rung claim again. */ + Assert.True(RungVersion < StorageVersion.SchemaVersion); Assert.Equal(versions.Distinct().OrderBy(v => v), versions); var above = versions.Where(v => v > 45).OrderBy(v => v).ToList(); @@ -127,23 +132,26 @@ which it is looking at. */ Assert.Contains($"reader.GetBoolean({ProbeOrdinal})", viewer, StringComparison.Ordinal); Assert.Contains("hasMuteRuleReloadBeacon", viewer, StringComparison.Ordinal); - Assert.Equal(StorageVersion.SchemaVersion, ViewerDataService.RequiredStoreSchemaVersion); - var method = typeof(ViewerDataService) .GetMethod("MapProbedSchemaVersion", BindingFlags.NonPublic | BindingFlags.Static)!; var arity = method.GetParameters().Length; - /* The top rung's sentinel IS the last argument. */ - Assert.Equal(ProbeOrdinal, arity - 1); + /* Not the top rung any more, so this sentinel is not the last argument. */ + Assert.True(ProbeOrdinal < arity - 1); + + /* A store migrated to exactly V117 — this rung's sentinel true and every LATER one false — maps to + 117. Everything above this ordinal is turned off, so further rungs do not have to touch this. */ + var toThisRung = Enumerable.Repeat((object)true, arity).ToArray(); + for (var i = ProbeOrdinal + 1; i < arity; i++) + { + toThisRung[i] = false; + } - /* Every sentinel true = a fully-migrated store, which must map to exactly this version. Built by - reflection so the arity tracks the signature. */ - var all = Enumerable.Repeat((object)true, arity).ToArray(); - Assert.Equal(StorageVersion.SchemaVersion, (int)method.Invoke(null, all)!); + Assert.Equal(RungVersion, (int)method.Invoke(null, toThisRung)!); - /* One rung behind: every sentinel EXCEPT this one reports 116 (the previous top rung). Without this - the arm above could be satisfied by an unconditional return and nothing would notice. */ - var behind = Enumerable.Repeat((object)true, arity).ToArray(); + /* One rung behind: this sentinel AND every later one false must report 116. Without it the arm + above could be satisfied by an unconditional return and nothing would notice. */ + var behind = (object[])toThisRung.Clone(); behind[ProbeOrdinal] = false; Assert.Equal(PreviousVersion, (int)method.Invoke(null, behind)!); } diff --git a/Darling/Darling.Tests/PgCpuCapacityHeadroomTests.cs b/Darling/Darling.Tests/PgCpuCapacityHeadroomTests.cs index 9bae74f8c..3b6e881f2 100644 --- a/Darling/Darling.Tests/PgCpuCapacityHeadroomTests.cs +++ b/Darling/Darling.Tests/PgCpuCapacityHeadroomTests.cs @@ -604,8 +604,15 @@ public void TheAlertTextNamesWhatEachPercentageIsAFractionOf() Assert.Contains("of currently allocated capacity", body, StringComparison.Ordinal); /* The metric NAMES are deliberately unchanged, for the engine-parity reason #2719 recorded: a mute - rule or history filter built on these strings keeps working across the fix. */ - Assert.Contains("\"High CPU\"", body, StringComparison.Ordinal); + rule or history filter built on these strings keeps working across the fix. + + #3282 made the fire name a REFERENCE to AlertEngine.CpuPersistenceMetric rather than a second + literal, which is a stronger form of the same claim: the two engines now use one string by + construction instead of two that happen to match, and the persistence-gate row is the same + subject on both. The literal itself is pinned where it now lives, in + AlertEngineTests.CpuGateDefaults_AreDerivedFromTheSampleCadence. */ + Assert.Contains("AlertEngine.CpuPersistenceMetric", body, StringComparison.Ordinal); + Assert.Equal("High CPU", PerformanceMonitor.Alerting.AlertEngine.CpuPersistenceMetric); Assert.Contains("\"CPU Resolved\"", body, StringComparison.Ordinal); } @@ -629,7 +636,7 @@ private static string EvaluatePgCpuBody() /* The slice has to be a method, not a fragment: an off-by-one on either bound silently shrinks it and every Assert.DoesNotContain above starts passing for the wrong reason. */ - Assert.Contains("var reading = await DarlingPgCpuUtilizationReader.GetLatestAsync", body, StringComparison.Ordinal); + Assert.Contains("await DarlingPgCpuUtilizationReader.GetSamplesSinceAsync", body, StringComparison.Ordinal); Assert.True(body.Length > 1500, $"the sliced body is only {body.Length} chars, which cannot be this method"); return body; diff --git a/Darling/Darling.Tests/RepoFileAdoptionTests.cs b/Darling/Darling.Tests/RepoFileAdoptionTests.cs index 59090796e..3c282a036 100644 --- a/Darling/Darling.Tests/RepoFileAdoptionTests.cs +++ b/Darling/Darling.Tests/RepoFileAdoptionTests.cs @@ -110,6 +110,7 @@ public sealed class RepoFileAdoptionTests /// private static readonly string[] s_lfReaders = { + "BuiltinAlertPersistenceRungTests.cs", "ChartWindowDomainTests.cs", "DarlingPathFilterGateTests.cs", "FleetCardCollectionStaleNamesItsPopulationTests.cs", diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs index 8acd5dfe3..5d963db5a 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs @@ -521,12 +521,21 @@ already living in PerformanceMonitor.Alerting - reusing it here is what keeps th reported by the process that fired it. */ private readonly ConcurrentDictionary _pgPoisonWaitCooldownSeeded = new(StringComparer.Ordinal); - /* #2719: same LIVE-STATE shape as Long-Running Query above — CPU is a continuous gauge, so a cooldown - timestamp and an active flag are enough; it does not need RollingCountAlertGate, which exists for + /* #2719: CPU is a continuous gauge, so it does not need RollingCountAlertGate, which exists for rolling-WINDOW COUNTS (Deadlocks/Blocking) where the same event can sit in the window across several - sweeps. Mirrors AlertEngine's own _activeCpuAlert/_lastCpuAlert shape for SQL Server's High CPU. */ + sweeps. The cooldown timestamp stays live-state, mirroring AlertEngine's own _lastCpuAlert. + + #3282: the active flag that used to sit beside it is GONE, replaced by the shared + AlertPersistenceGate's record — which carries the same "an incident is open" bit plus the streak + that earned it, and is PERSISTED. Both halves of that mattered here. The bool was in-memory only, + so a restart over a standing condition re-announced an incident the operator already had open; and + the streak has to live in the same value as the flag, because a caller that can advance one without + the other is a caller that can lose a signal. Seeded once per key from the same + config.alert_persistence_state row AlertEngine's SQL Server twin reads, under the same "High CPU" + metric name — the parity #2719 chose for the metric strings means no second row shape is needed. */ private readonly ConcurrentDictionary _lastPgCpuAlert = new(StringComparer.Ordinal); - private readonly ConcurrentDictionary _activePgCpuAlert = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary _pgCpuPersistence = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary _pgCpuPersistenceSeeded = new(StringComparer.Ordinal); /* #2716: none of the Postgres alerts' watermarks above survive a restart — AlertEngine seeds its own SQL Server twins of _lastAlertedPgDeadlockCount/_lastAlertedPgBlockingCount from @@ -3298,11 +3307,12 @@ plans were none of them evaluated. The snapshot already documents a null CPU pai degrading to (null, null) costs this tick its CPU alert and nothing else. */ double? sqlCpu = null; double? totalCpu = null; + DateTime? cpuSampleTime = null; var cpuReadClock = Stopwatch.StartNew(); try { - (sqlCpu, totalCpu) = await ReadLatestCpuAsync(runtime.ServerId, cancellationToken); + (sqlCpu, totalCpu, cpuSampleTime) = await ReadLatestCpuAsync(runtime.ServerId, cancellationToken); } catch (OperationCanceledException) { @@ -3326,7 +3336,10 @@ degrading to (null, null) costs this tick its CPU alert and nothing else. */ SqlCpuPercent: sqlCpu, TotalCpuPercent: totalCpu, IsAzureSqlDb: runtime.Target.IsAzureSqlDb, - Suppressed: false); + Suppressed: false, + /* #3282: the gate counts breaching SAMPLES, not sweeps — the sweep runs every 30 s and this + sample advances about once a minute, so without the instant a re-read would count twice. */ + CpuSampleTimeUtc: cpuSampleTime); await engine.EvaluateServerAsync(snapshot, cancellationToken); sweepReadClock.Restart(); @@ -3558,41 +3571,195 @@ private async Task EvaluatePgCpuAsync( if (!alertSettings.CpuEnabled) { + /* Not an observation: turning the feature off is not CPU recovering, so the gate is left + exactly as it stands and re-enabling resumes from the streak that was there. */ return; } - const string metricName = "High CPU"; + const string metricName = AlertEngine.CpuPersistenceMetric; var key = snapshot.ServerKey; + var stateStore = new PgAlertStateStore(_postgres, _logger); var readClock = Stopwatch.StartNew(); try { var now = DateTime.UtcNow; - var reading = await DarlingPgCpuUtilizationReader.GetLatestAsync(_postgres, runtime.ServerId, now, cancellationToken); + + /* #3282: seed the gate's record once per key, mirroring AlertEngine's own + EnsureWatermarksSeededAsync — the same reason #2716 seeds the deadlock/blocking watermarks + here. Without it a restart forgets an open incident and the first post-restart pass to fill + the streak announces it again. + + The metric name is the shared AlertEngine.CpuPersistenceMetric ("High CPU"), the same string + the mute context and the history row use, so this row is the same subject an operator sees in + config_alert_log. server_id never collides across engines, so one table serves both. */ + if (_pgCpuPersistenceSeeded.TryAdd(key, true)) + { + var seeded = await stateStore.LoadAlertPersistenceAsync(key, metricName); + readClock.Restart(); + if (seeded.HasValue) + { + _pgCpuPersistence[key] = seeded.Value; + + /* An ALREADY-OPEN incident gets its cooldown clock stamped, the same way + AlertEngine's own seeding does and for the same reason: the persisted Firing bit + stops a second rising edge, but _lastPgCpuAlert is in-memory, so an empty clock plus + a still-breaching condition would deliver the standing-condition reminder on the + first post-restart pass. Doing this on only one of the two engines would be the + shared-seam half-fix this whole issue is about, in miniature. */ + if (seeded.Value.State.Firing) + { + _lastPgCpuAlert[key] = now; + } + } + } + + var priorRecord = _pgCpuPersistence.TryGetValue(key, out var cached) + ? cached + : AlertPersistenceRecord.Initial; + + /* The BATCH of readings this pass has not counted yet, oldest first — not just the latest one. + Performance Insights is sampled at a 60-second period while pg_cpu_utilization is a + five-minute collector, so roughly five samples arrive together; counting only the newest + would discard four in five and make CpuBreachSamples take three batches (~15 minutes) + instead of three minutes. See DarlingPgCpuUtilizationReader.GetSamplesSinceAsync. */ + var samples = await DarlingPgCpuUtilizationReader.GetSamplesSinceAsync( + _postgres, runtime.ServerId, priorRecord.LastObservedSampleUtc, now, cancellationToken); readClock.Restart(); + var record = priorRecord; + + /* AT MOST ONE EDGE PER PASS. The loop stops at the first Fire or Resolve and leaves the rest of + the batch for the next sweep, 30 seconds later (s_alertSweepInterval) — not the collector's + five minutes, which is what makes this cheap. + + This is a STRUCTURAL fix rather than a guard, and it replaces one. Accumulating the batch's + edges into flags and deciding at the end loses information, and it lost it twice: a fire and + a resolve for the same incident flattened into "both happened", and then — the review catch — + a pre-existing incident's resolve overwritten by a later, unrelated fire in the same batch, + so the operator never heard that the incident they had open was over. Two instances, one + category: a SEQUENCE of edges compressed into a summary. Stopping at the first edge makes the + category unreachable instead of guarding its members, and it makes this pass the same shape as + AlertEngine's SQL Server twin, where one sweep is one observation and the fire and resolve + arms are mutually exclusive by construction. + + The cost is stated rather than hidden: after a restart or a collector gap, a whole excursion + can arrive as a fire and then a resolve one sweep apart. That is what happened — the condition + did hold for CpuBreachSamples samples and did then clear — and reporting both is the only + option that cannot drop an edge, which is the property that failed twice. In steady state the + 30-second sweep sees each sample on its own and this never arises. */ + var outcome = PersistenceOutcome.None; + DarlingPgCpuUtilizationReader.CpuSample? lastCounted = null; + double? lastCapacityPercent = null; + + foreach (var sample in samples) + { + /* #3281: percent of the CONFIGURED ACU ceiling, which is what the threshold means, reached + through the SAME shared decision the two fleet cards band on. Always the Performance + Insights arm, because this evaluator exists only for PostgreSQL targets. + + Read out into a local rather than compared inline: a lifted `double? >= int` is quietly + false on null, and "no capacity sample" deserves to be a named state rather than an + arithmetic accident. */ + var capacityPercent = FleetCpuProvenance.CpuBandInputPercent( + sample.CpuPercent, sample.AcuUtilizationPercent, FleetCpuSource.PerformanceInsights); + + if (!capacityPercent.HasValue) + { + /* NO CAPACITY READING FREEZES THE GATE — never a breach, never a clear. A missing + headroom figure is not a measurement of the thing the threshold is against, and the + pre-#3282 code treated it as "not exceeded", which resolved an open incident on the + strength of a reading it never took. Not firing on it was already the rule (#3281 + refuses to fall back to percent-of-allocated); not RESOLVING on it is the other half + of the same rule. + + `continue` rather than `break` because this is not an EDGE: ending the pass here + would let one capacity-less minute stall the whole batch and every sample behind it. + + The skipped sample is NOT revisited, and an earlier version of this comment claimed + otherwise (review catch). Once any later sample in the batch counts, the watermark + advances past this one and `sample_time > $2` excludes it for good — and it could not + be corrected anyway: RdsCpuIngestor COPYs new rows keyed off MAX(sample_time) and + never updates an inserted one, so there is no backfill path to wait for. Nothing is + lost by that, which is the point: a sample with no capacity figure has no + contribution to make to a gate counting breaches of a capacity threshold. */ + continue; + } + + var evaluation = AlertPersistenceGate.Evaluate( + record.State, + capacityPercent.Value >= alertSettings.CpuThresholdPercent, + AlertEngine.CpuBreachSamples, + AlertEngine.CpuClearSamples); + + record = new AlertPersistenceRecord(evaluation.State, sample.SampleTimeUtc); + lastCounted = sample; + lastCapacityPercent = capacityPercent; + + if (evaluation.Outcome != PersistenceOutcome.None) + { + outcome = evaluation.Outcome; + break; + } + } + + if (!record.Equals(priorRecord)) + { + _pgCpuPersistence[key] = record; + await stateStore.SaveAlertPersistenceAsync(key, metricName, record); + readClock.Restart(); + } + var cooldown = TimeSpan.FromMinutes(Math.Max(1, _alertCooldownMinutes)); - var wasActive = _activePgCpuAlert.TryGetValue(key, out var activeBefore) && activeBefore; - - /* #3281: percent of the CONFIGURED ACU ceiling, which is what the threshold means — reached - through the SAME shared decision the two fleet cards band on, so the alert and the card - cannot disagree about which figure puts a server in trouble. Always the Performance Insights - arm, because this evaluator exists only for PostgreSQL targets. - - Read out into a local rather than compared inline: a lifted `double? >= int` is quietly - false on null, and "no capacity sample" deserves to be a named state rather than an - arithmetic accident. */ - var capacityPercent = reading is null - ? null - : FleetCpuProvenance.CpuBandInputPercent( - reading.CpuPercent, reading.AcuUtilizationPercent, FleetCpuSource.PerformanceInsights); - var exceeded = capacityPercent.HasValue - && capacityPercent.Value >= alertSettings.CpuThresholdPercent; - _activePgCpuAlert[key] = exceeded; - - if (exceeded) - { - var cooldownElapsed = !_lastPgCpuAlert.TryGetValue(key, out var last) || now - last >= cooldown; + + /* THE REMINDER'S LEVEL IS THE LATEST KNOWN READING, not only a reading new to this pass — the + two are different questions and only the gate cares about newness. + + The batch is empty on most sweeps by design: the sweep is 30 seconds and pg_cpu_utilization + is a five-minute collector, so about nine sweeps in ten see no new sample. Deriving + `breaching` from the batch alone therefore skipped the standing-condition reminder on those + sweeps, which capped its cadence at the COLLECTOR interval instead of the configured + cooldown. Masked at the default 15-minute cooldown, silent at any cooldown shorter than five + minutes, and a parity break with AlertEngine.CheckCpuAsync — which computes `breaching` + unconditionally from the latest reading every sweep — of exactly the kind the comment below + claims does not exist. Review catch; the pre-batch code had this property for free because it + re-read the latest reading every sweep, and the batch rewrite dropped it on this engine only. + + So: fall back to that same single-latest read when the batch brought nothing, and use it for + the reminder decision ONLY. It never advances the gate and never moves the observed-sample + watermark, because it is not a new observation — it is the answer to "is the condition still + standing". At most one store read per sweep either way, which is what the pre-batch code + cost. And it is the read with the 15-minute freshness bound, so a collector that stops does + not leave the reminder firing forever on an hours-old reading. */ + if (!lastCapacityPercent.HasValue && record.State.Firing) + { + var standing = await DarlingPgCpuUtilizationReader.GetLatestAsync( + _postgres, runtime.ServerId, now, cancellationToken); + readClock.Restart(); + if (standing is not null) + { + lastCapacityPercent = FleetCpuProvenance.CpuBandInputPercent( + standing.CpuPercent, standing.AcuUtilizationPercent, FleetCpuSource.PerformanceInsights); + lastCounted = new DarlingPgCpuUtilizationReader.CpuSample( + standing.SampleTimeUtc, standing.CpuPercent, standing.AcuUtilizationPercent, + standing.ServerlessCapacityAcu, standing.MaxConfiguredAcu); + } + } + + bool breaching = lastCapacityPercent.HasValue + && lastCapacityPercent.Value >= alertSettings.CpuThresholdPercent; + + if (record.State.Firing && breaching) + { + /* The rising edge is cooldown-gated like every other fire, matching AlertEngine's SQL + Server twin exactly. #3282 changed what counts as an incident, deliberately not how the + cooldown works, and an engine-specific bypass here would make one threshold mean two + things across the two engines — the parity #2719 chose these metric names for. The gate + has already imposed CpuBreachSamples samples of delay, and a fire/resolve/fire cycle + needs CpuBreachSamples + CpuClearSamples samples, which at the ~60-second sample cadence + is about the default cooldown anyway. */ + var cooldownElapsed = !_lastPgCpuAlert.TryGetValue(key, out var last) + || now - last >= cooldown; if (!cooldownElapsed) { return; @@ -3608,7 +3775,8 @@ arithmetic accident. */ /* The ACU line only when both halves were sampled: "N of M ACU" with either missing would be a fabricated pair, and the percentage above already carries the answer. */ - var allocation = reading!.ServerlessCapacityAcu.HasValue && reading.MaxConfiguredAcu.HasValue + var reading = lastCounted!; + var allocation = reading.ServerlessCapacityAcu.HasValue && reading.MaxConfiguredAcu.HasValue ? $" Allocated: {reading.ServerlessCapacityAcu:0.#} of {reading.MaxConfiguredAcu:0.#} ACU\n" : string.Empty; @@ -3617,33 +3785,40 @@ await _alertDeliverer.DeliverAsync( key, snapshot.ServerName, metricName, - $"{capacityPercent!.Value:F0}%", + $"{lastCapacityPercent!.Value:F0}%", $"{alertSettings.CpuThresholdPercent}%", Context: null, /* Both figures, each labelled with what it is a fraction OF — the whole defect - #3281 names is a reader taking one for the other. */ + #3281 names is a reader taking one for the other. The sustained-for line is + #3282's: without it a reader cannot tell this from the single-sample alert that + used to arrive here, and "it has been this way for three samples" is most of + what makes the message worth acting on. */ DetailText: - $" Capacity: {capacityPercent.Value:F0}% {FleetCpuProvenance.CapacityDenominator}\n" + $" Capacity: {lastCapacityPercent.Value:F0}% {FleetCpuProvenance.CapacityDenominator}\n" + allocation + $" Instance CPU: {reading.CpuPercent:F0}% of currently allocated capacity\n" - + $" Threshold: {alertSettings.CpuThresholdPercent}%", - NumericCurrentValue: capacityPercent.Value, + + $" Threshold: {alertSettings.CpuThresholdPercent}%\n" + + $" Sustained: {AlertEngine.CpuBreachSamples} consecutive samples", + NumericCurrentValue: lastCapacityPercent.Value, NumericThresholdValue: alertSettings.CpuThresholdPercent, Muted: muted, Severity: null, ShortMessage: - $"Capacity at {capacityPercent.Value:F0}% {FleetCpuProvenance.CapacityDenominator} " + $"Capacity at {lastCapacityPercent.Value:F0}% {FleetCpuProvenance.CapacityDenominator} " + $"(threshold: {alertSettings.CpuThresholdPercent}%)"), cancellationToken); readClock.Restart(); } - else if (wasActive) + else if (outcome == PersistenceOutcome.Resolve) { - /* Edge-triggered, so this has to fire even when the capacity reading went away — and it - says which of the two happened rather than claiming a recovery it did not measure. */ + /* The falling edge is the gate's, after CpuClearSamples consecutive clears, rather than the + first sample under the bar. It still says which of the two happened rather than claiming + a recovery it did not measure — a subject whose capacity readings stopped arriving + altogether never reaches this arm at all now, because a missing reading freezes the gate + instead of clearing it. */ await NotifyPgResolutionAsync(key, snapshot.ServerName, metricName, "CPU Resolved", - capacityPercent.HasValue - ? $"{snapshot.ServerName}: capacity back to {capacityPercent.Value:F0}% " + lastCapacityPercent.HasValue + ? $"{snapshot.ServerName}: capacity back to {lastCapacityPercent.Value:F0}% " + FleetCpuProvenance.CapacityDenominator : $"{snapshot.ServerName}: no current capacity reading, so the alert is cleared"); } @@ -4429,7 +4604,7 @@ the same way a real backend id already is on its own. */ /// same-row-across-offsets behaviour are both pinned by test. /// internal const string LatestCpuSql = @" -SELECT sqlserver_cpu_utilization, other_process_cpu_utilization +SELECT sqlserver_cpu_utilization, other_process_cpu_utilization, sample_time FROM cpu_utilization_stats WHERE server_id = $1 ORDER BY collection_time DESC, sample_time DESC @@ -4441,10 +4616,11 @@ FROM cpu_utilization_stats /// ServerSummaryItem.TotalCpuPercent derivation (:140-141): total = SQL + (other ?? 0), /// null when there is no SQL sample (Azure SQL DB stores other as 0; Linux stores NULL). /// - private async Task<(double? SqlCpu, double? TotalCpu)> ReadLatestCpuAsync(int serverId, CancellationToken cancellationToken) + private async Task<(double? SqlCpu, double? TotalCpu, DateTime? SampleTime)> ReadLatestCpuAsync(int serverId, CancellationToken cancellationToken) { double? sqlCpu = null; double? otherCpu = null; + DateTime? sampleTime = null; await using var connection = await _postgres!.OpenConnectionAsync(cancellationToken); using var command = new NpgsqlCommand( @@ -4456,10 +4632,15 @@ FROM cpu_utilization_stats { sqlCpu = reader.IsDBNull(0) ? null : Convert.ToDouble(reader.GetValue(0), CultureInfo.InvariantCulture); otherCpu = reader.IsDBNull(1) ? null : Convert.ToDouble(reader.GetValue(1), CultureInfo.InvariantCulture); + /* #3282: the sample's own instant, which is the persistence gate's observation identity. Left + Kind=Unspecified as it comes off the naive-UTC `timestamp` column — it is only ever compared + against the value this same read stored last sweep, so coercing it to Kind=Utc would shift + one side of that comparison by the host's offset and, east of UTC, freeze the gate. */ + sampleTime = reader.IsDBNull(2) ? null : reader.GetDateTime(2); } double? totalCpu = sqlCpu.HasValue ? sqlCpu.Value + (otherCpu ?? 0) : null; - return (sqlCpu, totalCpu); + return (sqlCpu, totalCpu, sampleTime); } /// diff --git a/Darling/PerformanceMonitor.Darling.Service/PgAlertStateStore.cs b/Darling/PerformanceMonitor.Darling.Service/PgAlertStateStore.cs index 6fe8e3405..27db57824 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PgAlertStateStore.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PgAlertStateStore.cs @@ -380,6 +380,95 @@ a missed or duplicated alert. The alert itself has already been decided by the g } } + /// + /// #3282: loads one subject's built-in persistence-gate record from the V117 + /// config.alert_persistence_state table. + /// + /// Returns null on failure, which the engine reads as "no memory" and arms the gate from zero — + /// the same degradation a host with no persistence at all gets. Crucially that is a DELAY (the streak + /// rebuilds over the next few samples), never a re-announcement: a load failure cannot resurrect a + /// firing bit it did not read, so the worst outcome is one extra fire after the gate refills. + /// + public async Task LoadAlertPersistenceAsync(string serverKey, string metricName) + { + try + { + await using var connection = await _postgres.OpenConnectionAsync(); + using var command = new NpgsqlCommand(@" +SELECT consecutive_breaches, consecutive_clears, firing, last_observed_sample_at +FROM config.alert_persistence_state +WHERE server_id = $1 +AND metric_name = $2", connection) { CommandTimeout = DarlingAlertReadAdapter.AlertPassCommandTimeoutSeconds }; + command.Parameters.AddWithValue(ParseServerKey(serverKey)); + command.Parameters.AddWithValue(metricName); + + await using var reader = await command.ExecuteReaderAsync(); + if (!await reader.ReadAsync()) + { + return null; + } + + /* The sample instant comes back Kind=Unspecified from a `timestamp` column and is naive UTC by + the store-wide convention. Deliberately NOT coerced to Kind=Utc: it is only ever COMPARED + against the sample instant the alert pass read out of the same store, on the same convention, + and a ToUniversalTime() here would shift it by the host's offset — which on a host east of UTC + would make every incoming sample look OLDER than the stored one and freeze the gate forever. */ + return new AlertPersistenceRecord( + new PersistenceState(reader.GetInt32(0), reader.GetInt32(1), reader.GetBoolean(2)), + reader.IsDBNull(3) ? null : reader.GetDateTime(3)); + } + catch (Exception ex) + { + _logger?.LogError("Could not load the alert persistence gate ({Metric}): {Message}", metricName, ex.Message); + return null; + } + } + + /// + /// #3282: upserts one subject's persistence-gate record. Same posture as the watermark writes — a + /// dropped write costs the streak across a restart, never an alert, because the gate has already + /// decided this observation from the engine's in-memory record. + /// + public async Task SaveAlertPersistenceAsync(string serverKey, string metricName, AlertPersistenceRecord record) + { + try + { + await using var connection = await _postgres.OpenConnectionAsync(); + using var command = new NpgsqlCommand(@" +INSERT INTO config.alert_persistence_state + (server_id, metric_name, consecutive_breaches, consecutive_clears, firing, last_observed_sample_at, updated_at) +VALUES ($1, $2, $3, $4, $5, $6, $7) +ON CONFLICT (server_id, metric_name) DO UPDATE SET + consecutive_breaches = EXCLUDED.consecutive_breaches, + consecutive_clears = EXCLUDED.consecutive_clears, + firing = EXCLUDED.firing, + last_observed_sample_at = EXCLUDED.last_observed_sample_at, + updated_at = EXCLUDED.updated_at", connection) { CommandTimeout = DarlingAlertReadAdapter.AlertPassCommandTimeoutSeconds }; + command.Parameters.AddWithValue(ParseServerKey(serverKey)); + command.Parameters.AddWithValue(metricName); + command.Parameters.AddWithValue(record.State.ConsecutiveBreaches); + command.Parameters.AddWithValue(record.State.ConsecutiveClears); + command.Parameters.AddWithValue(record.State.Firing); + command.Parameters.Add(new NpgsqlParameter + { + NpgsqlDbType = NpgsqlTypes.NpgsqlDbType.Timestamp, + /* Kind-stripped for the same reason every other timestamp bind here is: Npgsql does NOT + reject Kind=Utc against `timestamp`, it infers timestamptz and PostgreSQL casts into the + SERVER's zone — storing a value offset from the sample instants this is compared against. */ + Value = record.LastObservedSampleUtc.HasValue + ? Naive(record.LastObservedSampleUtc.Value) + : (object)DBNull.Value, + }); + command.Parameters.AddWithValue(NaiveUtcNow()); + + await command.ExecuteNonQueryAsync(); + } + catch (Exception ex) + { + _logger?.LogError("Could not persist the alert persistence gate ({Metric}): {Message}", metricName, ex.Message); + } + } + /// Naive-UTC now, Kind-Unspecified — the product's PG timestamp discipline. private static DateTime NaiveUtcNow() => DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified); diff --git a/Darling/PerformanceMonitor.Darling.Storage/DarlingPgCpuUtilizationReader.cs b/Darling/PerformanceMonitor.Darling.Storage/DarlingPgCpuUtilizationReader.cs index 59fab48c3..6c19e30fb 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/DarlingPgCpuUtilizationReader.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/DarlingPgCpuUtilizationReader.cs @@ -100,6 +100,75 @@ than 0 (a 0 here would read as measured headroom). */ reader.IsDBNull(4) ? null : reader.GetDouble(4)); } + /// + /// The most samples one persistence-gate pass will consume (#3282). Performance Insights is queried at + /// a 60-second period and pg_cpu_utilization is collected every five minutes, so a normal batch + /// is about five samples; bounds how far back the pass looks, and this bounds + /// how many rows it will carry out of that window if the collector caught up after a longer gap. The + /// gate's counters saturate at their thresholds, so a larger batch could not change the outcome — this + /// only stops one pass reading an unbounded list. + /// + public const int GateBatchLimit = 32; + + internal const string SamplesSinceSql = """ + SELECT sample_time, cpu_percent, acu_utilization_percent, serverless_capacity_acu, max_configured_acu + FROM pg_cpu_utilization + WHERE server_id = $1 + AND sample_time > $2 + AND cpu_percent IS NOT NULL + ORDER BY sample_time + LIMIT $3 + """; + + /// + /// Every reading newer than , OLDEST FIRST, for the High CPU persistence + /// gate (#3282). + /// + /// Why the gate cannot just re-read the latest row here, the way the SQL Server side does. + /// On SQL Server the alert sweep (30 s) is faster than the sample (about 60 s), so every sample is seen + /// by at least one sweep and "latest row, counted once" loses nothing. Performance Insights is the + /// opposite shape: pg_cpu_utilization is a five-minute collector ingesting 60-second data points, + /// so five samples land at once and four of every five would never be counted. Requiring three + /// consecutive breaching samples would then take three BATCHES — about fifteen minutes — which is a + /// saturation event reported long after it mattered. Reading the batch makes three samples mean three + /// minutes on both engines, which is also what lets one threshold keep meaning one thing across them. + /// + /// is floored at nowUtc - : a subject + /// with no memory, or one whose collector was away for an hour, considers only readings recent enough to + /// describe "right now" — the same bound applies, so the gate and the + /// single-reading path agree on what counts as current. + /// + public static async Task> GetSamplesSinceAsync( + NpgsqlDataSource postgres, int serverId, DateTime? afterUtc, DateTime nowUtc, + CancellationToken cancellationToken = default) + { + ArgumentNullException.ThrowIfNull(postgres); + + var floor = nowUtc - Freshness; + var after = afterUtc.HasValue && afterUtc.Value > floor ? afterUtc.Value : floor; + + var samples = new System.Collections.Generic.List(); + await using var command = postgres.CreateCommand(SamplesSinceSql); + command.CommandTimeout = StorageCommandDeadlines.McpReadSeconds; + command.Parameters.AddWithValue(serverId); + /* Naive UTC at the bind, like every other comparison against the naive `timestamp` columns. */ + command.Parameters.AddWithValue(DateTime.SpecifyKind(after, DateTimeKind.Unspecified)); + command.Parameters.AddWithValue(GateBatchLimit); + + await using var reader = await command.ExecuteReaderAsync(cancellationToken); + while (await reader.ReadAsync(cancellationToken)) + { + samples.Add(new CpuSample( + DateTime.SpecifyKind(reader.GetDateTime(0), DateTimeKind.Utc), + reader.GetDouble(1), + reader.IsDBNull(2) ? null : reader.GetDouble(2), + reader.IsDBNull(3) ? null : reader.GetDouble(3), + reader.IsDBNull(4) ? null : reader.GetDouble(4))); + } + + return samples; + } + internal const string HistorySql = """ SELECT sample_time, cpu_percent, acu_utilization_percent, serverless_capacity_acu, max_configured_acu FROM pg_cpu_utilization diff --git a/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs b/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs index 71bc5fc29..972275bf6 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/PgMigrations.cs @@ -174,6 +174,7 @@ here costs a fresh-through-this-rung store nothing and rung 54's own copy no-ops new Migration(115, "pg-cpu-capacity-headroom", V115Sql), new Migration(116, "custom-alert-core", V116Sql), new Migration(117, "mute-rules-reload-beacon", V117Sql), + new Migration(118, "builtin-alert-persistence", V118Sql), }; /// @@ -342,6 +343,47 @@ CREATE TRIGGER trg_bump_mute_rules AFTER INSERT OR UPDATE OR DELETE ON config.config_mute_rules FOR EACH STATEMENT EXECUTE FUNCTION config.config_bump_version();"; + /// + /// V118 — the BUILT-IN alert catalog's persistence-gate state (#3282). One additive config-plane table, + /// the twin of V116's custom_alert_state for alerts nobody authored. + /// + /// Why a separate table rather than the custom one. custom_alert_state is keyed + /// (rule_id, server_id) with a foreign key to custom_alert_rules and + /// ON DELETE CASCADE; a built-in alert has no rule row to reference, so it has no key there and + /// nothing to cascade from. The built-in subject is (server_id, metric_name) — the key the + /// engine's other state already uses. + /// + /// And why not columns on config_edge_trigger_watermarks, which carries exactly that + /// key. Two reasons, each sufficient. That column is one monotonic integer documented as "the highest + /// already-alerted rolling-window count", and a resettable pair of counters is not that shape. And Lite's + /// twin of the row is written with INSERT OR REPLACE over a PARTIAL column list, which resets + /// every unlisted column to its default — a streak living there would zero itself on every fired + /// blocking or deadlock alert, i.e. while it was being counted. Same finding V61's + /// incident_occurrences was split out for. + /// + /// last_observed_sample_at is the gate's observation identity, not display data. The gate + /// counts consecutive breaching SAMPLES and the alert sweep is twice as fast as a CPU sample arrives, so + /// without it a re-read of one sample would count as a second observation and "three consecutive + /// breaches" would be satisfied inside ninety seconds — shorter than every excursion #3282 measured. + /// + /// No config_bump_version trigger, like V116: this is evaluator state written every new + /// sample, and a reload beacon on it would force a fleet-wide ReloadFromStoreAsync once a minute + /// per server. No per-table GRANT either — provisioning re-runs + /// GRANT … ON ALL TABLES IN SCHEMA config after migration — and no viewer/mcp read yet: the + /// service's alert pass is the only consumer. + /// + private const string V118Sql = @" +CREATE TABLE IF NOT EXISTS config.alert_persistence_state ( + server_id integer NOT NULL, + metric_name text NOT NULL, + consecutive_breaches integer NOT NULL DEFAULT 0, + consecutive_clears integer NOT NULL DEFAULT 0, + firing boolean NOT NULL DEFAULT FALSE, + last_observed_sample_at timestamp, + updated_at timestamp NOT NULL DEFAULT (now() AT TIME ZONE 'UTC'), + PRIMARY KEY (server_id, metric_name) +);"; + /// /// V2 — the service's observability store: the servers registry (upserted on every /// successful connect) and the per-run collection_log. Column names deliberately mirror diff --git a/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs b/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs index bcf067318..9f209c101 100644 --- a/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs +++ b/Darling/PerformanceMonitor.Darling.Storage/StorageVersion.cs @@ -16,5 +16,5 @@ namespace PerformanceMonitor.Darling.Storage; /// public static class StorageVersion { - public const int SchemaVersion = 117; + public const int SchemaVersion = 118; } diff --git a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs index 4c9b05e1e..22c21e384 100644 --- a/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs +++ b/Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.cs @@ -757,7 +757,8 @@ report the rung absent on a fully-migrated store and the connect gate would refu EXISTS (SELECT 1 FROM pg_trigger t JOIN pg_class c ON c.oid = t.tgrelid JOIN pg_namespace n ON n.oid = c.relnamespace - WHERE t.tgname = 'trg_bump_mute_rules' AND n.nspname = 'config')"; + WHERE t.tgname = 'trg_bump_mute_rules' AND n.nspname = 'config'), + EXISTS (SELECT 1 FROM information_schema.tables WHERE table_name = 'alert_persistence_state')"; /// The store schema version this viewer build requires — the highest migration it knows /// (). The connect-time gate blocks a store below this. @@ -779,7 +780,7 @@ report the rung absent on a fully-migrated store and the connect gate would refu await using var reader = await command.ExecuteReaderAsync(cancellationToken); if (await reader.ReadAsync(cancellationToken)) { - return MapProbedSchemaVersion(reader.GetBoolean(0), reader.GetBoolean(1), reader.GetBoolean(2), reader.GetBoolean(3), reader.GetBoolean(4), reader.GetBoolean(5), reader.GetBoolean(6), reader.GetBoolean(7), reader.GetBoolean(8), reader.GetBoolean(9), reader.GetBoolean(10), reader.GetBoolean(11), reader.GetBoolean(12), reader.GetBoolean(13), reader.GetBoolean(14), reader.GetBoolean(15), reader.GetBoolean(16), reader.GetBoolean(17), reader.GetBoolean(18), reader.GetBoolean(19), reader.GetBoolean(20), reader.GetBoolean(21), reader.GetBoolean(22), reader.GetBoolean(23), reader.GetBoolean(24), reader.GetBoolean(25), reader.GetBoolean(26), reader.GetBoolean(27), reader.GetBoolean(28), reader.GetBoolean(29), reader.GetBoolean(30), reader.GetBoolean(31), reader.GetBoolean(32), reader.GetBoolean(33), reader.GetBoolean(34), reader.GetBoolean(35), reader.GetBoolean(36), reader.GetBoolean(37), reader.GetBoolean(38), reader.GetBoolean(39), reader.GetBoolean(40), reader.GetBoolean(41), reader.GetBoolean(42), reader.GetBoolean(43), reader.GetBoolean(44), reader.GetBoolean(45), reader.GetBoolean(46), reader.GetBoolean(47), reader.GetBoolean(48), reader.GetBoolean(49), reader.GetBoolean(50), reader.GetBoolean(51), reader.GetBoolean(52), reader.GetBoolean(53), reader.GetBoolean(54), reader.GetBoolean(55), reader.GetBoolean(56), reader.GetBoolean(57), reader.GetBoolean(58), reader.GetBoolean(59), reader.GetBoolean(60), reader.GetBoolean(61), reader.GetBoolean(62), reader.GetBoolean(63), reader.GetBoolean(64), reader.GetBoolean(65), reader.GetBoolean(66), reader.GetBoolean(67), reader.GetBoolean(68), reader.GetBoolean(69), reader.GetBoolean(70), reader.GetBoolean(71), reader.GetBoolean(72), reader.GetBoolean(73), reader.GetBoolean(74), reader.GetBoolean(75), reader.GetBoolean(76), reader.GetBoolean(77), reader.GetBoolean(78), reader.GetBoolean(79), reader.GetBoolean(80), reader.GetBoolean(81), reader.GetBoolean(82), reader.GetBoolean(83), reader.GetBoolean(84), reader.GetBoolean(85), reader.GetBoolean(86), reader.GetBoolean(87), reader.GetBoolean(88), reader.GetBoolean(89), reader.GetBoolean(90), reader.GetBoolean(91), reader.GetBoolean(92)); + return MapProbedSchemaVersion(reader.GetBoolean(0), reader.GetBoolean(1), reader.GetBoolean(2), reader.GetBoolean(3), reader.GetBoolean(4), reader.GetBoolean(5), reader.GetBoolean(6), reader.GetBoolean(7), reader.GetBoolean(8), reader.GetBoolean(9), reader.GetBoolean(10), reader.GetBoolean(11), reader.GetBoolean(12), reader.GetBoolean(13), reader.GetBoolean(14), reader.GetBoolean(15), reader.GetBoolean(16), reader.GetBoolean(17), reader.GetBoolean(18), reader.GetBoolean(19), reader.GetBoolean(20), reader.GetBoolean(21), reader.GetBoolean(22), reader.GetBoolean(23), reader.GetBoolean(24), reader.GetBoolean(25), reader.GetBoolean(26), reader.GetBoolean(27), reader.GetBoolean(28), reader.GetBoolean(29), reader.GetBoolean(30), reader.GetBoolean(31), reader.GetBoolean(32), reader.GetBoolean(33), reader.GetBoolean(34), reader.GetBoolean(35), reader.GetBoolean(36), reader.GetBoolean(37), reader.GetBoolean(38), reader.GetBoolean(39), reader.GetBoolean(40), reader.GetBoolean(41), reader.GetBoolean(42), reader.GetBoolean(43), reader.GetBoolean(44), reader.GetBoolean(45), reader.GetBoolean(46), reader.GetBoolean(47), reader.GetBoolean(48), reader.GetBoolean(49), reader.GetBoolean(50), reader.GetBoolean(51), reader.GetBoolean(52), reader.GetBoolean(53), reader.GetBoolean(54), reader.GetBoolean(55), reader.GetBoolean(56), reader.GetBoolean(57), reader.GetBoolean(58), reader.GetBoolean(59), reader.GetBoolean(60), reader.GetBoolean(61), reader.GetBoolean(62), reader.GetBoolean(63), reader.GetBoolean(64), reader.GetBoolean(65), reader.GetBoolean(66), reader.GetBoolean(67), reader.GetBoolean(68), reader.GetBoolean(69), reader.GetBoolean(70), reader.GetBoolean(71), reader.GetBoolean(72), reader.GetBoolean(73), reader.GetBoolean(74), reader.GetBoolean(75), reader.GetBoolean(76), reader.GetBoolean(77), reader.GetBoolean(78), reader.GetBoolean(79), reader.GetBoolean(80), reader.GetBoolean(81), reader.GetBoolean(82), reader.GetBoolean(83), reader.GetBoolean(84), reader.GetBoolean(85), reader.GetBoolean(86), reader.GetBoolean(87), reader.GetBoolean(88), reader.GetBoolean(89), reader.GetBoolean(90), reader.GetBoolean(91), reader.GetBoolean(92), reader.GetBoolean(93)); } return null; @@ -804,7 +805,7 @@ report the rung absent on a fully-migrated store and the connect gate would refu /// is unit-tested without a live store; any schema bump past the newest arm trips the pinning test that keeps /// this in step with . /// - internal static int MapProbedSchemaVersion(bool hasConfigControlPlane, bool hasAlertDeliveryOverride, bool hasAnalysisState, bool hasAlertTuningKnobs, bool hasDefaultTraceEvents, bool hasIndexObjectStatsLatestIndex, bool hasCollectionLogHypertableOrPlainPg, bool hasJobHistory, bool hasAgentStatus, bool hasGenericWebhook, bool hasDeadlocksDatabaseName, bool hasQueryStoreReplicaRole, bool hasLongQueryCompletions, bool hasWebDashboardConfig, bool hasCustomViews, bool hasServerTags, bool hasConnectionRefireKnobs = false, bool hasAgCollectors = false, bool hasAgAlertKnobs = false, bool hasAgLatencyColumns = false, bool hasAgDisconnectRefire = false, bool hasPayloadDimensions = false, bool hasDimFloorIndexes = false, bool hasBlockingWaitThreshold = false, bool hasQueryStoreIntervalIdentity = false, bool hasPagerDutyWebhook = false, bool hasPagerDutyProxy = false, bool hasCollectorState = false, bool hasPlanCorrection = false, bool hasPvsStats = false, bool hasPvsPressureKnobs = false, bool hasDatabaseStateAlert = false, bool hasServerTagColour = false, bool hasQueryStatsHostObject = false, bool hasFindingDrillDown = false, bool hasStoreMetrics = false, bool hasPlanDimGzip = false, bool hasSelfAlertKnobs = false, bool hasJobMetricsColumns = false, bool hasJobCadenceKnob = false, bool hasBackfillSwitch = false, bool hasCollectorMemoryKnobs = false, bool hasDatabaseStateEdgeMemory = false, bool hasIncidentOccurrences = false, bool hasPlanXmlCompressionKnob = false, bool hasMonitoredServerEngine = false, bool hasPgBlockingEdges = false, bool hasQueryStorePlanMap = false, bool hasPgStatementText = false, bool hasQueryStoreText = false, bool hasPlanContentRetentionKnob = false, bool hasQueryStoreHealth = false, bool hasQueryStoreTextHash = false, bool hasComposeTimeoutKnob = false, bool hasFileGrowthAlert = false, bool hasCollectionLogFanoutRollup = false, bool hasTempDbMaxSize = false, bool hasServerEngineKind = false, bool hasPgDatabaseStats = false, bool hasPgIndexUsageStats = false, bool hasPgTableBloatStats = false, bool hasPgSessionStates = false, bool hasPgPlanCaptureReadiness = false, bool hasPgWriteStats = false, bool hasPgExtensionAvailability = false, bool hasPgLockStats = false, bool hasPgColumnStats = false, bool hasPgReplicationStats = false, bool hasPgBufferUsage = false, bool hasPgIndexBloat = false, bool hasPgPerDatabaseAttribution = false, bool hasPgWaitSampling = false, bool hasPgKernelStats = false, bool hasPgPredicateStats = false, bool hasPgPlanCapture = false, bool hasPgMajorVersion = false, bool hasPg18IoBytes = false, bool hasPgServerConfig = false, bool hasPgDeadlocks = false, bool hasPgDeadlockIdentity = false, bool hasCollectorCost = false, bool hasPgCpuUtilization = false, bool hasPlanForceActions = false, bool hasCollectionLogPhaseSplit = false, bool hasCollectionLogDrainForensics = false, bool hasCollectionLogFetchPhaseSums = false, bool hasStoreLogSelfMonitoring = false, bool hasCollectorStallProbes = false, bool hasRemediationCredentialAndActor = false, bool hasPgIndexBloatEstimate = false, bool hasPgCpuCapacityHeadroom = false, bool hasCustomAlertCore = false, bool hasMuteRuleReloadBeacon = false) + internal static int MapProbedSchemaVersion(bool hasConfigControlPlane, bool hasAlertDeliveryOverride, bool hasAnalysisState, bool hasAlertTuningKnobs, bool hasDefaultTraceEvents, bool hasIndexObjectStatsLatestIndex, bool hasCollectionLogHypertableOrPlainPg, bool hasJobHistory, bool hasAgentStatus, bool hasGenericWebhook, bool hasDeadlocksDatabaseName, bool hasQueryStoreReplicaRole, bool hasLongQueryCompletions, bool hasWebDashboardConfig, bool hasCustomViews, bool hasServerTags, bool hasConnectionRefireKnobs = false, bool hasAgCollectors = false, bool hasAgAlertKnobs = false, bool hasAgLatencyColumns = false, bool hasAgDisconnectRefire = false, bool hasPayloadDimensions = false, bool hasDimFloorIndexes = false, bool hasBlockingWaitThreshold = false, bool hasQueryStoreIntervalIdentity = false, bool hasPagerDutyWebhook = false, bool hasPagerDutyProxy = false, bool hasCollectorState = false, bool hasPlanCorrection = false, bool hasPvsStats = false, bool hasPvsPressureKnobs = false, bool hasDatabaseStateAlert = false, bool hasServerTagColour = false, bool hasQueryStatsHostObject = false, bool hasFindingDrillDown = false, bool hasStoreMetrics = false, bool hasPlanDimGzip = false, bool hasSelfAlertKnobs = false, bool hasJobMetricsColumns = false, bool hasJobCadenceKnob = false, bool hasBackfillSwitch = false, bool hasCollectorMemoryKnobs = false, bool hasDatabaseStateEdgeMemory = false, bool hasIncidentOccurrences = false, bool hasPlanXmlCompressionKnob = false, bool hasMonitoredServerEngine = false, bool hasPgBlockingEdges = false, bool hasQueryStorePlanMap = false, bool hasPgStatementText = false, bool hasQueryStoreText = false, bool hasPlanContentRetentionKnob = false, bool hasQueryStoreHealth = false, bool hasQueryStoreTextHash = false, bool hasComposeTimeoutKnob = false, bool hasFileGrowthAlert = false, bool hasCollectionLogFanoutRollup = false, bool hasTempDbMaxSize = false, bool hasServerEngineKind = false, bool hasPgDatabaseStats = false, bool hasPgIndexUsageStats = false, bool hasPgTableBloatStats = false, bool hasPgSessionStates = false, bool hasPgPlanCaptureReadiness = false, bool hasPgWriteStats = false, bool hasPgExtensionAvailability = false, bool hasPgLockStats = false, bool hasPgColumnStats = false, bool hasPgReplicationStats = false, bool hasPgBufferUsage = false, bool hasPgIndexBloat = false, bool hasPgPerDatabaseAttribution = false, bool hasPgWaitSampling = false, bool hasPgKernelStats = false, bool hasPgPredicateStats = false, bool hasPgPlanCapture = false, bool hasPgMajorVersion = false, bool hasPg18IoBytes = false, bool hasPgServerConfig = false, bool hasPgDeadlocks = false, bool hasPgDeadlockIdentity = false, bool hasCollectorCost = false, bool hasPgCpuUtilization = false, bool hasPlanForceActions = false, bool hasCollectionLogPhaseSplit = false, bool hasCollectionLogDrainForensics = false, bool hasCollectionLogFetchPhaseSums = false, bool hasStoreLogSelfMonitoring = false, bool hasCollectorStallProbes = false, bool hasRemediationCredentialAndActor = false, bool hasPgIndexBloatEstimate = false, bool hasPgCpuCapacityHeadroom = false, bool hasCustomAlertCore = false, bool hasMuteRuleReloadBeacon = false, bool hasBuiltinAlertPersistence = false) { /* V71 (the PostgreSQL blocking-edges rung): a table-existence sentinel and now the newest-first arm. A collector table would ordinarily get no arm at all — see the V63-V69 note below — but the TOP @@ -953,6 +954,26 @@ information_schema lines but cannot strip a comment. */ StorageVersion.SchemaVersion (116) rather than falling through to 115 and showing a spurious upgrade banner on a store that is current. The table is named only in the probe line, not this prose, per the V71 finding (the coverage ratchet strips information_schema lines but cannot strip a comment). */ + /* V118 (#3282): config.alert_persistence_state — the BUILT-IN alert catalog's persistence-gate + state. Table-existence sentinel, newest-first, and now the TOP rung, so a fully-migrated store + maps to EXACTLY StorageVersion.SchemaVersion (118) rather than falling through to 117 and showing + a spurious upgrade banner on a store that is current. + + The reason to gate is that standing invariant rather than a viewer read that would throw: nothing + in the viewer reads this table — the SERVICE's alert pass is its only consumer. What the banner + buys on the way is worth having anyway. On a store still at 117 the CPU alert has no memory of + an open incident across a service restart, so every High CPU row in that history was produced by + the pre-#3282 single-sample rule, and an operator comparing alert volume before and after should + know which rule wrote which rows before drawing a conclusion from the drop. + + The table is named only in the probe line, not this prose, per the V71 finding: the coverage + ratchet strips information_schema lines but cannot strip a comment, so a prose mention would + exempt it. */ + if (hasBuiltinAlertPersistence) + { + return 118; + } + /* V117 (#3315): config.config_mute_rules joins the config_version reload beacon, so a mute write of any kind makes the service re-load its in-memory mute cache on the next sweep. Trigger-existence sentinel — the trigger is the only object the rung creates — and newest-first, so it sits above diff --git a/Lite.Tests/DuckDbSchemaTests.cs b/Lite.Tests/DuckDbSchemaTests.cs index 691e949a5..ef1fe3dd4 100644 --- a/Lite.Tests/DuckDbSchemaTests.cs +++ b/Lite.Tests/DuckDbSchemaTests.cs @@ -160,8 +160,10 @@ Includes config_edge_trigger_watermarks (#1145), config_incident_occurrences (#2 (#1962 per-server state a collector's own rows cannot produce), plan_correction (#1952 automatic plan correction), pvs_stats (#1951 ADR persistent version store), the database-state alert's database_states collector + config_database_state_expected - control table, and the fleet-tag tables server_tags + server_tag_map (#2020 2b-i). */ - Assert.Equal(54, tableCount); + control table, the fleet-tag tables server_tags + server_tag_map (#2020 2b-i), and + config_alert_persistence_state (#3282's built-in persistence-gate state, the twin of + Darling's config.alert_persistence_state). */ + Assert.Equal(55, tableCount); } [Fact] diff --git a/Lite.Tests/LiteAlertForwardingTests.cs b/Lite.Tests/LiteAlertForwardingTests.cs index 70ee7d523..eb92542aa 100644 --- a/Lite.Tests/LiteAlertForwardingTests.cs +++ b/Lite.Tests/LiteAlertForwardingTests.cs @@ -201,6 +201,21 @@ public Task> LoadIncidentOc ? states : new Dictionary(StringComparer.Ordinal)); + + /* #3282: REAL persistence, not a no-op — the gate's whole point is that it survives a restart, and + a fake that forgot the record would let a broken seed path pass. Keyed like both real stores. */ + public Dictionary<(string Key, string Metric), AlertPersistenceRecord> Persistence { get; } = new(); + public List<(string Key, string Metric, AlertPersistenceRecord Record)> SavedPersistence { get; } = new(); + + public Task LoadAlertPersistenceAsync(string serverKey, string metricName) => + Task.FromResult(Persistence.TryGetValue((serverKey, metricName), out var r) ? (AlertPersistenceRecord?)r : null); + + public Task SaveAlertPersistenceAsync(string serverKey, string metricName, AlertPersistenceRecord record) + { + Persistence[(serverKey, metricName)] = record; + SavedPersistence.Add((serverKey, metricName, record)); + return Task.CompletedTask; + } public Task SaveIncidentOccurrencesAsync(string serverKey, string metricName, IReadOnlyDictionary states) { var replacement = new Dictionary(StringComparer.Ordinal); @@ -264,10 +279,20 @@ private sealed class Harness public DateTime? FailedJobWatermark() => StateStore.FailedJobWatermarks.TryGetValue(Key, out var w) ? w : (DateTime?)null; + /* #3282: distinct, increasing CPU sample instants by default — the realistic case, and the only + default that does not quietly put every test on a degraded path (see the same helper in + AlertEngineTests for the two ways a fixed or null default would lie). */ + private static int s_sampleTick; + + /// The instant distinct sample times are counted from — only the ordering matters. + public static readonly DateTime SampleBase = new(2026, 7, 1, 0, 0, 0, DateTimeKind.Utc); + public static AlertServerSnapshot Snapshot( double? sqlCpu = null, double? totalCpu = null, - bool isOnline = true, bool isAzureSqlDb = false, bool suppressed = false) => - new(Key, Name, isOnline, sqlCpu, totalCpu, isAzureSqlDb, suppressed); + bool isOnline = true, bool isAzureSqlDb = false, bool suppressed = false, + DateTime? cpuSampleTime = null) => + new(Key, Name, isOnline, sqlCpu, totalCpu, isAzureSqlDb, suppressed, + cpuSampleTime ?? SampleBase.AddMinutes(System.Threading.Interlocked.Increment(ref s_sampleTick))); } /// Everything except the named check off, so a scenario pins exactly one alert. @@ -310,6 +335,94 @@ private static void DisableAllChecks() 1. CPU fire → resolve strings (old MainWindow.AlertEngine.cs:64-116) ===================================================================================== */ + /// + /// Drives sweeps with DISTINCT, increasing CPU sample instants — one #3282 + /// gate observation per call. Returns the last instant so a caller can continue the sequence. + /// + private static async Task DriveCpuAsync( + AlertEngine engine, double? sqlCpu, double? totalCpu, int samples, DateTime from) + { + var at = from; + for (var i = 0; i < samples; i++) + { + at = at.AddMinutes(1); + await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: sqlCpu, totalCpu: totalCpu, cpuSampleTime: at)); + } + + return at; + } + + /// The suppressed twin — suppression is evaluate-but-don't-deliver, so the gate still + /// advances and a pin about suppression has to drive enough samples to reach the bar. + private static async Task DriveCpuSuppressedAsync( + AlertEngine engine, double? sqlCpu, double? totalCpu, int samples, DateTime from) + { + var at = from; + for (var i = 0; i < samples; i++) + { + at = at.AddMinutes(1); + await engine.EvaluateServerAsync( + Harness.Snapshot(sqlCpu: sqlCpu, totalCpu: totalCpu, suppressed: true, cpuSampleTime: at)); + } + + return at; + } + + [Fact] + public async Task Cpu_OneSampleOverTheBar_DoesNotFire_OnLiteEither() + { + /* The #3282 defect on the LITE side specifically. AlertEngine is shared, so the arithmetic is + covered by AlertEngineTests — what this pins is that Lite reaches the same behaviour through + its own snapshot and its own IAlertStateStore, which is the standing trap on every shared seam + here: a change landed only Darling-side leaves Lite reading a permanently-empty value. */ + DisableAllChecks(); + App.AlertCpuEnabled = true; + var h = new Harness(); + var engine = h.Build(); + + await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 99, samples: 1, from: Harness.SampleBase); + Assert.Empty(h.Deliverer.Outcomes); + + await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 99, samples: AlertEngine.CpuBreachSamples - 1, from: Harness.SampleBase.AddMinutes(1)); + Assert.Single(h.Deliverer.Outcomes); + } + + [Fact] + public async Task Cpu_LiteStateStore_PersistsTheStreakUnderTheSharedMetricName() + { + /* Lite's InMemoryStateStore stands in for LiteAlertStateStore over DuckDB. What matters is that + the engine writes the gate's record through the SEAM on the Lite path at all, and under the same + (server, metric) key Darling uses — so the two SKUs' rows are the same subject and a future + cross-store reader sees one shape. */ + DisableAllChecks(); + App.AlertCpuEnabled = true; + var h = new Harness(); + + await DriveCpuAsync(h.Build(), sqlCpu: 70, totalCpu: 95, samples: AlertEngine.CpuBreachSamples - 1, from: Harness.SampleBase); + + var persisted = Assert.Single(h.StateStore.Persistence); + Assert.Equal(AlertEngine.CpuPersistenceMetric, persisted.Key.Metric); + Assert.Equal(AlertEngine.CpuBreachSamples - 1, persisted.Value.State.ConsecutiveBreaches); + Assert.False(persisted.Value.State.Firing); + Assert.NotNull(persisted.Value.LastObservedSampleUtc); + } + + [Fact] + public async Task Cpu_LiteSchemaCarriesThePersistenceTable() + { + /* The Lite half of the store parity, asserted against the generator rather than a live DuckDB: + #3282 is useless on Lite if the table the state store writes does not exist. */ + Assert.Contains( + "config_alert_persistence_state", + PerformanceMonitorLite.Database.Schema.CreateAlertPersistenceStateTable, + StringComparison.Ordinal); + Assert.Contains( + PerformanceMonitorLite.Database.Schema.CreateAlertPersistenceStateTable, + PerformanceMonitorLite.Database.Schema.GetAllTableStatements()); + + await Task.CompletedTask; + } + [Fact] public async Task Cpu_FireAndResolve_CarriesTheOldLoopsExactStrings() { @@ -318,8 +431,11 @@ public async Task Cpu_FireAndResolve_CarriesTheOldLoopsExactStrings() var h = new Harness(); var engine = h.Build(); - /* Fire: Total mode uses TotalCpuPercent (:65 CpuPercentForAlert → Total). */ - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 92)); + /* Fire: Total mode uses TotalCpuPercent (:65 CpuPercentForAlert → Total), after + AlertEngine.CpuBreachSamples distinct samples over the bar (#3282 — one is no longer an + incident, on either SKU). The delivered STRINGS are what this test is about and they are + unchanged. */ + var at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 92, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase); var fired = Assert.Single(h.Deliverer.Outcomes); Assert.Equal("High CPU", fired.MetricName); @@ -338,9 +454,10 @@ public async Task Cpu_FireAndResolve_CarriesTheOldLoopsExactStrings() Assert.Equal(80d, fired.NumericThresholdValue); Assert.False(fired.Muted); - /* Resolve: :110-113 — exact title + message strings, Success-severity tray-only toast. */ + /* Resolve: :110-113 — exact title + message strings, Success-severity tray-only toast, after + AlertEngine.CpuClearSamples consecutive clears. */ h.Now = h.Now.AddMinutes(6); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 10, totalCpu: 12)); + await DriveCpuAsync(engine, sqlCpu: 10, totalCpu: 12, samples: AlertEngine.CpuClearSamples, from: at); var res = Assert.Single(h.Resolutions); Assert.Equal("CPU Resolved", res.Title); @@ -357,7 +474,7 @@ public async Task Cpu_SqlOnlyMode_UsesSqlValueAndLabel() var h = new Harness(); /* SqlOnly compares CpuPercent (90), not Total (95) — :65 CpuPercentForAlert → SqlOnly. */ - await h.Build().EvaluateServerAsync(Harness.Snapshot(sqlCpu: 90, totalCpu: 95)); + await DriveCpuAsync(h.Build(), sqlCpu: 90, totalCpu: 95, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase); var fired = Assert.Single(h.Deliverer.Outcomes); Assert.Equal("90% (SQL CPU)", fired.CurrentValue); @@ -435,7 +552,11 @@ public async Task SuppressedSweep_DeliversNothing_AndDoesNotAdvanceBlockingWater var engine = h.Build(); h.Adapter.Blocking.Add(BlockingRow(51)); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 95, totalCpu: 99, suppressed: true)); + /* CpuBreachSamples distinct samples, all suppressed (#3282 — the CPU gate advances under + suppression, exactly like the blocking gate, because suppression is evaluate-but-don't-deliver. + Driving only one sample would leave this pin unable to distinguish "suppressed" from "the streak + never reached the bar", which is the thing it exists to check). */ + var at = await DriveCpuSuppressedAsync(engine, sqlCpu: 95, totalCpu: 99, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase); Assert.Empty(h.Deliverer.Outcomes); Assert.Empty(h.Resolutions); @@ -443,7 +564,10 @@ public async Task SuppressedSweep_DeliversNothing_AndDoesNotAdvanceBlockingWater persisted; when the user un-acknowledges, the same lingering report still alerts. */ Assert.Empty(h.StateStore.SavedEdge); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 95, totalCpu: 99)); + /* One UNsuppressed sample is enough for CPU now: the gate is already firing from the suppressed + streak above, so this is the standing-condition delivery rather than a fresh rising edge — which + is the suppressed-gates-still-advance semantics this test is about. */ + await DriveCpuAsync(engine, sqlCpu: 95, totalCpu: 99, samples: 1, from: at); Assert.Equal(2, h.Deliverer.Outcomes.Count); /* CPU + blocking both fire once unsuppressed */ } @@ -461,13 +585,13 @@ public async Task MutedAlert_IsDeliveredFlaggedMuted_SoTheHistoryRowIsStillWritt var h = new Harness { Muted = true }; var engine = h.Build(); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 92)); + var at = await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 92, samples: AlertEngine.CpuBreachSamples, from: Harness.SampleBase); var fired = Assert.Single(h.Deliverer.Outcomes); Assert.True(fired.Muted); /* The cooldown was stamped even when muted (:76-78) — no second delivery inside it. */ h.Now = h.Now.AddMinutes(2); - await engine.EvaluateServerAsync(Harness.Snapshot(sqlCpu: 70, totalCpu: 92)); + await DriveCpuAsync(engine, sqlCpu: 70, totalCpu: 92, samples: 1, from: at); Assert.Single(h.Deliverer.Outcomes); } diff --git a/Lite/Database/DuckDbInitializer.cs b/Lite/Database/DuckDbInitializer.cs index 7aa9bb137..d4222db0d 100644 --- a/Lite/Database/DuckDbInitializer.cs +++ b/Lite/Database/DuckDbInitializer.cs @@ -271,7 +271,7 @@ public void Dispose() /// /// Current schema version. Increment this when schema changes require table rebuilds. /// - internal const int CurrentSchemaVersion = 57; + internal const int CurrentSchemaVersion = 58; private readonly string _archivePath; @@ -1548,6 +1548,34 @@ positional appender and old parquet are unaffected. */ } } } + + if (fromVersion < 58) + { + /* v58 (#3282): the built-in alert catalog's persistence-gate state, porting Darling's V117. + Before this no built-in alert required its condition to PERSIST — one sample over the bar + fired and the next sample under it resolved — so a momentary CPU spike was indistinguishable + from sustained saturation. New table only; fresh installs get it from + GetAllTableStatements() and this CREATE is for an existing database, idempotent so a re-run + is a no-op. + + Nothing to backfill, and nothing that could be: an absent row means "no streak and no open + incident", which is exactly what every server looked like before the gate existed. The first + few sweeps after the upgrade build the streak, so the first post-upgrade High CPU arrives + once the condition has actually held. + + Non-fatal, matching v54's posture: without the table the load returns null and the gate + lives for one process lifetime — a restart re-arms it from zero rather than breaking the + alert path. */ + _logger?.LogInformation("Running migration to v58: adding config_alert_persistence_state"); + try + { + await ExecuteNonQueryAsync(connection, Schema.CreateAlertPersistenceStateTable); + } + catch (Exception ex) + { + _logger?.LogWarning("Migration to v58 encountered an error (non-fatal): {Error}", ex.Message); + } + } } /// diff --git a/Lite/Database/Schema.cs b/Lite/Database/Schema.cs index 3a53cfbbf..de0d1d0b6 100644 --- a/Lite/Database/Schema.cs +++ b/Lite/Database/Schema.cs @@ -118,6 +118,34 @@ last_observed_at is not display data — it is what makes a row's staleness deci when the incident ends, but a crash mid-incident strands one, and a stranded row trusted on that fingerprint's NEXT incident would decay its already-counted mark to the new window count and report the recurrence as nothing new. */ + /* The BUILT-IN alert catalog's persistence-gate state (#3282), the twin of Darling's + config.alert_persistence_state (PgMigrations V117) — same columns, same key. "How long must this + condition hold before it counts" for the gauge alerts: consecutive breaching samples so far, + consecutive clearing samples so far, and whether an incident is currently open. + + A SEPARATE table rather than columns on config_edge_trigger_watermarks, for the two reasons + config_incident_occurrences was split out for and which hold independently here too. That column is + one monotonic integer meaning "the highest already-alerted rolling-window count", and a resettable + counter pair is not that shape. And the watermark row is written with INSERT OR REPLACE over a + PARTIAL column list, which resets every unlisted column to its default — a streak living there would + zero itself on every fired blocking or deadlock alert, i.e. exactly while it was being counted. + + last_observed_sample_at is the gate's observation identity, not display data. The gate counts + consecutive breaching SAMPLES while the sweep runs twice as often as a CPU sample arrives, so + without it a re-read of one sample would count as a second observation and the streak would fill + from data that never changed. */ + public const string CreateAlertPersistenceStateTable = @" +CREATE TABLE IF NOT EXISTS config_alert_persistence_state ( + server_id INTEGER NOT NULL, + metric_name VARCHAR NOT NULL, + consecutive_breaches INTEGER NOT NULL, + consecutive_clears INTEGER NOT NULL, + firing BOOLEAN NOT NULL, + last_observed_sample_at TIMESTAMP, + updated_at TIMESTAMP NOT NULL, + PRIMARY KEY (server_id, metric_name) +)"; + public const string CreateIncidentOccurrencesTable = @" CREATE TABLE IF NOT EXISTS config_incident_occurrences ( server_id INTEGER NOT NULL, @@ -238,6 +266,7 @@ public static IEnumerable GetAllTableStatements() yield return CreateAlertLogTable; yield return CreateEdgeTriggerWatermarksTable; yield return CreateIncidentOccurrencesTable; + yield return CreateAlertPersistenceStateTable; yield return CreateCollectorStateTable; yield return CreateMuteRulesTable; yield return CreateDismissedArchiveAlertsTable; diff --git a/Lite/MainWindow.AlertEngine.cs b/Lite/MainWindow.AlertEngine.cs index f6d3fd7f5..f33bf22d8 100644 --- a/Lite/MainWindow.AlertEngine.cs +++ b/Lite/MainWindow.AlertEngine.cs @@ -82,7 +82,11 @@ only a dictionary lookup. */ SqlCpuPercent: summary.CpuPercent, TotalCpuPercent: summary.TotalCpuPercent, IsAzureSqlDb: connStatus?.SqlEngineEdition == 5, - Suppressed: suppressPopups); + Suppressed: suppressPopups, + /* #3282: the gate counts breaching CPU SAMPLES rather than sweeps. Lite's sweep is the + 30-second overview timer and the ring-buffer sample behind CpuPercent advances about once a + minute, so without the instant one sample would fill the streak on its own. */ + CpuSampleTimeUtc: summary.CpuSampleTime); AlertSweepResult sweep; try diff --git a/Lite/Services/DuckDbAlertHistoryStore.cs b/Lite/Services/DuckDbAlertHistoryStore.cs index caf6c463f..8224dba65 100644 --- a/Lite/Services/DuckDbAlertHistoryStore.cs +++ b/Lite/Services/DuckDbAlertHistoryStore.cs @@ -528,6 +528,133 @@ gate has already decided by this point. */ } } + /// + /// #3282: loads one subject's built-in persistence-gate record from config_alert_persistence_state + /// — the Lite twin of Darling's V117 table. Returns null when there is no row, which the engine reads as + /// "no memory" and arms the gate from zero. + /// + /// Null on failure too, deliberately. A load failure cannot resurrect a firing bit it did + /// not read, so the worst outcome is the streak rebuilding over the next few samples — a delay, never a + /// re-announcement of an incident the user already has open. + /// + public async Task<(int Breaches, int Clears, bool Firing, DateTime? LastObservedSampleUtc)?> + LoadAlertPersistenceAsync(int serverId, string metricName) + { + try + { + var duckDb = _duckDb; + if (duckDb == null) + { + var dbPath = App.DatabasePath; + if (string.IsNullOrEmpty(dbPath)) return null; + duckDb = new DuckDbInitializer(dbPath); + } + + using var readLock = duckDb.AcquireReadLock(); + using var connection = duckDb.CreateConnection(); + await connection.OpenAsync(); + + using var command = connection.CreateCommand(); + command.CommandText = @" +SELECT consecutive_breaches, consecutive_clears, firing, last_observed_sample_at +FROM config_alert_persistence_state +WHERE server_id = $1 +AND metric_name = $2"; + command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = serverId }); + command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = metricName }); + + using var reader = await command.ExecuteReaderAsync(); + if (!await reader.ReadAsync()) + { + return null; + } + + return ( + Convert.ToInt32(reader.GetValue(0)), + Convert.ToInt32(reader.GetValue(1)), + Convert.ToBoolean(reader.GetValue(2)), + reader.IsDBNull(3) ? (DateTime?)null : Convert.ToDateTime(reader.GetValue(3))); + } + catch (Exception ex) + { + AppLogger.Error("Alerts", $"Could not load the alert persistence gate ({metricName}): {ex.Message}"); + return null; + } + } + + /// + /// #3282: upserts one subject's persistence-gate record. + /// + /// DELETE-then-INSERT rather than INSERT OR REPLACE, and that is the point rather than a + /// style choice: the partial-column INSERT OR REPLACE used on + /// config_edge_trigger_watermarks resets every unlisted column to its default, which is precisely + /// why this state could not live on that table. Naming every column on one statement here would work + /// today and would silently zero whatever a later column adds, so the shape that cannot rot is the one + /// that writes the whole row. One transaction, because a delete that commits without its insert is a + /// subject that forgot it had an incident open. + /// + /// Failures are absorbed like the watermark writes: the gate has already decided this observation + /// from the engine's in-memory record, so a dropped write costs the streak across a restart and never an + /// alert. + /// + public async Task SaveAlertPersistenceAsync( + int serverId, string metricName, int breaches, int clears, bool firing, DateTime? lastObservedSampleUtc) + { + try + { + var duckDb = _duckDb; + if (duckDb == null) + { + var dbPath = App.DatabasePath; + if (string.IsNullOrEmpty(dbPath)) return; + duckDb = new DuckDbInitializer(dbPath); + } + + using var writeLock = duckDb.AcquireWriteLock(); + using var connection = duckDb.CreateConnection(); + await connection.OpenAsync(); + using var transaction = connection.BeginTransaction(); + + using (var prune = connection.CreateCommand()) + { + prune.Transaction = transaction; + prune.CommandText = @" +DELETE FROM config_alert_persistence_state +WHERE server_id = $1 +AND metric_name = $2"; + prune.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = serverId }); + prune.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = metricName }); + await prune.ExecuteNonQueryAsync(); + } + + using (var insert = connection.CreateCommand()) + { + insert.Transaction = transaction; + insert.CommandText = @" +INSERT INTO config_alert_persistence_state + (server_id, metric_name, consecutive_breaches, consecutive_clears, firing, last_observed_sample_at, updated_at) +VALUES ($1, $2, $3, $4, $5, $6, $7)"; + insert.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = serverId }); + insert.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = metricName }); + insert.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = breaches }); + insert.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = clears }); + insert.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = firing }); + insert.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter + { + Value = lastObservedSampleUtc.HasValue ? lastObservedSampleUtc.Value : (object)DBNull.Value, + }); + insert.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = DateTime.UtcNow }); + await insert.ExecuteNonQueryAsync(); + } + + transaction.Commit(); + } + catch (Exception ex) + { + AppLogger.Error("Alerts", $"Could not persist the alert persistence gate ({metricName}): {ex.Message}"); + } + } + /* The failed-Agent-job watermark shares the edge-trigger table but is time-based, not a count: it holds the newest already-alerted failure's server-local run time (stored in watermark_time, not the INTEGER watermark column). One reserved metric_name row per server. */ diff --git a/Lite/Services/LiteAlertStateStore.cs b/Lite/Services/LiteAlertStateStore.cs index 67d877925..c06a746ab 100644 --- a/Lite/Services/LiteAlertStateStore.cs +++ b/Lite/Services/LiteAlertStateStore.cs @@ -179,6 +179,43 @@ public Task SaveIncidentOccurrencesAsync( return Task.Run(() => _store.SaveIncidentOccurrencesAsync(serverId, metricName, rows)); } + /// + /// #3282: the built-in persistence-gate record, over config_alert_persistence_state — Lite's twin + /// of Darling's V117 table, so the shared engine's CPU gate behaves the same on both SKUs. Wrapped in + /// Task.Run like every other method here: DuckDB.NET's I/O is synchronous under its async facade + /// and the engine runs on the WPF dispatcher, so an unwrapped call is a UI hitch (#1202). + /// + public Task LoadAlertPersistenceAsync(string serverKey, string metricName) + { + var serverId = ParseServerKey(serverKey); + return Task.Run(async () => + { + var row = await _store.LoadAlertPersistenceAsync(serverId, metricName); + if (row is null) + { + return (AlertPersistenceRecord?)null; + } + + return new AlertPersistenceRecord( + new PersistenceState(row.Value.Breaches, row.Value.Clears, row.Value.Firing), + row.Value.LastObservedSampleUtc); + }); + } + + /// #3282: upserts the persistence-gate record — see the store method for why it writes the + /// whole row rather than an INSERT OR REPLACE over a partial column list. + public Task SaveAlertPersistenceAsync(string serverKey, string metricName, AlertPersistenceRecord record) + { + var serverId = ParseServerKey(serverKey); + return Task.Run(() => _store.SaveAlertPersistenceAsync( + serverId, + metricName, + record.State.ConsecutiveBreaches, + record.State.ConsecutiveClears, + record.State.Firing, + record.LastObservedSampleUtc)); + } + private static int ParseServerKey(string serverKey) => int.Parse(serverKey, CultureInfo.InvariantCulture); } diff --git a/Lite/Services/LocalDataService.Overview.cs b/Lite/Services/LocalDataService.Overview.cs index 13313605c..03871f0c3 100644 --- a/Lite/Services/LocalDataService.Overview.cs +++ b/Lite/Services/LocalDataService.Overview.cs @@ -26,6 +26,7 @@ public partial class LocalDataService double? cpuPercent = null; double? otherProcessCpuPercent = null; + DateTime? cpuSampleTime = null; double? memoryMb = null; int blockingCount = 0; int deadlockCount = 0; @@ -48,6 +49,11 @@ ORDER BY sample_time DESC cpuPercent = reader.IsDBNull(0) ? null : ToDouble(reader.GetValue(0)); otherProcessCpuPercent = reader.IsDBNull(1) ? null : ToDouble(reader.GetValue(1)); lastCollection = reader.IsDBNull(2) ? null : reader.GetDateTime(2); + /* #3282: the SAME value, kept under its own name because the two have different jobs. + lastCollection is overwritten below by the collection_log read and is freshness display; + this one is the CPU persistence gate's observation identity and must stay the instant of + the CPU reading these percentages came from. */ + cpuSampleTime = lastCollection; } } @@ -117,6 +123,7 @@ FROM v_collection_log ServerId = serverId, CpuPercent = cpuPercent, OtherProcessCpuPercent = otherProcessCpuPercent, + CpuSampleTime = cpuSampleTime, MemoryMb = memoryMb, BlockingCount = blockingCount, DeadlockCount = deadlockCount, @@ -269,6 +276,14 @@ public class ServerSummaryItem public double? CpuPercent { get; set; } /// Non-SQL-Server CPU on the host (computed as 100 - SystemIdle - ProcessUtilization). NULL on Azure SQL DB. public double? OtherProcessCpuPercent { get; set; } + + /// + /// The sample_time of the CPU reading came from (#3282) — the shared + /// engine's persistence-gate observation identity, NOT display data. Distinct from + /// , which is the newest collection of anything and is what the + /// freshness band is computed from. + /// + public DateTime? CpuSampleTime { get; set; } /// Total non-idle CPU on the host = sql_server + other_process. Tracks closer to OS user+system counters. public double? TotalCpuPercent => CpuPercent.HasValue ? CpuPercent.Value + (OtherProcessCpuPercent ?? 0) : null; diff --git a/PerformanceMonitor.Alerting/AlertEngine.cs b/PerformanceMonitor.Alerting/AlertEngine.cs index 523e84245..de1a15937 100644 --- a/PerformanceMonitor.Alerting/AlertEngine.cs +++ b/PerformanceMonitor.Alerting/AlertEngine.cs @@ -126,7 +126,14 @@ can lag the alert cooldown (PerformanceMonitor's own dogfooding on _lastPoisonWaitCollectionTime = new(); private readonly ConcurrentDictionary _lastLongRunningQueryAlert = new(); private readonly ConcurrentDictionary _lastTempDbSpaceAlert = new(); @@ -139,7 +146,18 @@ condition. Gate re-fire on BOTH the cooldown AND a newer collection_time than la /* Active-condition flags driving the resolved/cleared transitions — Lite's MainWindow.xaml.cs:78-89. */ - private readonly ConcurrentDictionary _activeCpuAlert = new(); + + /* #3282: CPU's active flag is GONE from this family and replaced by the persistence gate's record, + which carries the same "an incident is open" bit plus the streak that earned it. Two reasons it + could not stay a bool here. It has to survive a restart — an in-memory flag meant the first + post-restart sweep over a standing condition re-announced an incident the operator already had open + — and the streak has to live in the same value as the flag, because a caller that can advance one + without the other is a caller that can lose a signal. + + Seeded once per key from IAlertStateStore alongside the watermarks, then written through on change. + The cache is authoritative WITHIN the process: EvaluateServerAsync serializes per server, so the + read-modify-write below cannot interleave for one key. */ + private readonly ConcurrentDictionary _cpuPersistence = new(); private readonly ConcurrentDictionary _activeBlockingAlert = new(); private readonly ConcurrentDictionary _activeBlockingWaitAlert = new(); private readonly ConcurrentDictionary _activeDeadlockAlert = new(); @@ -352,6 +370,33 @@ private async Task EnsureWatermarksSeededAsync(string key, CancellationToken ct) { _lastAlertedFailedJobTime[key] = failedJob.Value; } + + /* #3282: the CPU persistence gate's record, seeded here for the same reason the watermarks + are — so the first post-restart sweep knows an incident is already open (and does not + re-announce it) and resumes the streak instead of restarting it. A store with no row hands + back null and the subject starts at AlertPersistenceRecord.Initial, which is also what a + host with no persistence gets; either way the gate arms from zero rather than misfiring. */ + readClock.Restart(); + var cpuPersistence = await _stateStore.LoadAlertPersistenceAsync(key, CpuPersistenceMetric); + if (cpuPersistence.HasValue) + { + _cpuPersistence[key] = cpuPersistence.Value; + + /* An incident that was ALREADY OPEN gets its cooldown clock stamped as if it had just been + announced. The persisted Firing bit stops the gate producing a second rising edge, but + the cooldown dictionaries are in-memory by design (see their field comment), so an empty + clock plus a still-breaching condition would deliver the standing-condition REMINDER on + the first post-restart sweep — an identical High CPU message seconds after a restart, + which is a re-announcement whatever it is called internally. Stamping makes the + reminder wait a full cooldown, which is what an operator who already has the incident + open would expect. Nothing can be lost: if the condition is still breaching when the + cooldown elapses the reminder fires then, and if it cleared while the service was down + the first ClearSamples clear samples resolve it. */ + if (cpuPersistence.Value.State.Firing) + { + _lastCpuAlert[key] = _utcNow(); + } + } } catch (OperationCanceledException) when (ct.IsCancellationRequested) { @@ -368,6 +413,50 @@ private async Task EnsureWatermarksSeededAsync(string key, CancellationToken ct) /* ---------------- CPU (Lite AlertEngine.cs:62-114) ---------------- */ + /// + /// Consecutive breaching CPU SAMPLES required before High CPU is an incident (#3282). Three, which is + /// about three minutes: the SQL Server figure comes off the SCHEDULER_MONITOR ring buffer, whose + /// entries are about a minute apart (CpuUtilizationCollector's own #2749 note measures "a real + /// ~60s gap to the next"), and the cpu_utilization collector is scheduled every minute. + /// + /// Derived from the measured excursion lengths, not picked. Every High CPU / CPU Resolved + /// pair delivered on a 42-server fleet in the 24 hours to 2026-09-11 was between 42 and 147 seconds + /// long, median 87 s; the worst of them held at or above the 80% bar for exactly two consecutive + /// one-minute samples before falling back to a 20% baseline. #3282 measured the same shape + /// independently (55 s and 87 s on SQL Server, a 30-second pair, and one target at 99% back to 23% + /// inside two minutes). Three samples is therefore the first bar above all of it, and the shortest + /// excursion that could still be TRUE when a human opens the message — which is the only thing that + /// makes a CPU page actionable rather than archaeology. + /// + /// Deliberately a constant rather than a setting, following PostgresAlertEvaluator's + /// stated position for its own thresholds: the product adds configuration when someone wants a + /// different number, not speculatively, and a knob here means a new config_alert_settings + /// column, a migration, a required member, Settings-window work and + /// MCP plumbing. Half of that is worse than none: #3314 is open precisely because a + /// delivery-governing number exists in the store and is unreachable from + /// get_alert_settings/update_alert_settings, and adding a second such number is the one + /// outcome to avoid. Public so it can be cited and pinned, and so raising it is a one-line diff. + /// + public const int CpuBreachSamples = 3; + + /// + /// Consecutive clearing CPU samples required to resolve an open High CPU incident (#3282). Two, and + /// deliberately fewer than : the two costs are not symmetric. A late + /// resolve leaves a stale open incident, which is mildly annoying; an early resolve announces a + /// recovery that the next sample contradicts, and a resolve/fire pair is exactly the noise this issue + /// exists to remove. Two is the smallest value that survives one sample dipping under the bar during a + /// real saturation event (#3282's "fired at 99% and resolved to 23% within two minutes" is that shape); + /// three would hold incidents open a further minute and buy nothing. + /// + public const int CpuClearSamples = 2; + + /// + /// The (server, metric) key the CPU gate's state is persisted under. The SAME string the mute context, + /// the history row and the resolve all use, so an operator reading config_alert_log and an + /// operator reading the persistence table are looking at one metric, not two spellings of it. + /// + public const string CpuPersistenceMetric = "High CPU"; + private async Task CheckCpuAsync( AlertServerSnapshot snapshot, string key, string serverName, DateTime now, TimeSpan alertCooldown, bool suppressed, CancellationToken ct) @@ -379,13 +468,75 @@ private async Task CheckCpuAsync( ? (snapshot.TotalCpuPercent ?? snapshot.SqlCpuPercent) : snapshot.SqlCpuPercent; string cpuMetricLabel = _settings.CpuAlertMode == CpuAlertMode.TotalServer ? "Total CPU" : "SQL CPU"; /* :64 */ - bool cpuExceeded = _settings.CpuEnabled - && alertCpuValue.HasValue - && alertCpuValue.Value >= _settings.CpuThresholdPercent; /* :65-67 */ - if (cpuExceeded) + if (!_settings.CpuEnabled) + { + /* The disabled case is not an observation and never was: flipping the feature off is not the + CPU recovering, so the gate is left exactly as it stands and no resolve is announced (the + pre-#3282 code reached the same outcome through its own _settings.CpuEnabled guard on the + resolve arm). Re-enabling resumes from the streak that was there. */ + return; + } + + if (!alertCpuValue.HasValue) + { + /* NO-DATA FREEZES THE GATE — never a breach, never a clear. The pre-#3282 code fell through to + its resolve arm here, so a CPU sample that simply went missing announced a recovery nobody + measured, rendered ": Total CPU back to %" because the value it interpolated was + null. Freezing is the same call CustomAlertEvaluator makes on a null scalar, and the same + reason every per-check catch in this class logs and skips: resolving on absent evidence + fabricates a recovery exactly as firing on it fabricates an alert. */ + return; + } + + var priorRecord = _cpuPersistence.TryGetValue(key, out var cached) ? cached : AlertPersistenceRecord.Initial; + + /* An observation counts only when it is a sample this subject has not counted yet. The sweep runs + on s_alertSweepInterval (30 s) while the ring-buffer sample behind alertCpuValue advances about + once a minute, so without this the SAME sample would advance the streak on consecutive sweeps + and CpuBreachSamples would be reached inside 90 seconds — shorter than every excursion #3282 + measured, i.e. the defect intact behind a gate that looked like it fixed it. + + A null sample instant counts every sweep instead (see AlertServerSnapshot.CpuSampleTimeUtc): the + persistence is then weaker, but the alert still fires, and silence is the one failure a monitoring + product cannot distinguish from health. + + Where two samples land between sweeps the older one is skipped, so a sustained excursion can need + one extra sample to reach the bar. That undercounts and therefore UNDER-fires, which is the + correct direction for an alert that pages — the same reasoning PostgresAlertEvaluator's + poison-wait window states for its own partial coverage. */ + bool freshSample = !snapshot.CpuSampleTimeUtc.HasValue + || !priorRecord.LastObservedSampleUtc.HasValue + || snapshot.CpuSampleTimeUtc.Value > priorRecord.LastObservedSampleUtc.Value; + + bool breaching = alertCpuValue.Value >= _settings.CpuThresholdPercent; /* :65-67 */ + var outcome = PersistenceOutcome.None; + + if (freshSample) + { + var evaluation = AlertPersistenceGate.Evaluate( + priorRecord.State, breaching, CpuBreachSamples, CpuClearSamples); + outcome = evaluation.Outcome; + + var nextRecord = new AlertPersistenceRecord( + evaluation.State, snapshot.CpuSampleTimeUtc ?? priorRecord.LastObservedSampleUtc); + + /* Value equality on the record is what makes the write skippable: on the vast majority of + sweeps nothing about the subject moved, and a store write per server per sweep would be + 42 pointless upserts a minute on the measured fleet. */ + if (!nextRecord.Equals(priorRecord)) + { + _cpuPersistence[key] = nextRecord; + await SaveCpuPersistenceAsync(key, nextRecord); + } + } + + bool incidentOpen = _cpuPersistence.TryGetValue(key, out var current) + ? current.State.Firing + : priorRecord.State.Firing; + + if (incidentOpen && breaching) { - _activeCpuAlert[key] = true; /* :71 */ if (!suppressed && CooldownElapsed(_lastCpuAlert, key, now, alertCooldown)) /* :72 */ { var muteCtx = new AlertMuteContext { ServerName = serverName, MetricName = "High CPU" }; /* :74 */ @@ -398,8 +549,8 @@ private async Task CheckCpuAsync( server-name prefix. The numerics are REQUIRED, not optional (#1830): the ported no-numerics form left the history stores parsing "87% (Total CPU)", which fails on the parenthesized label, so every High CPU row stored current_value 0 in Lite AND - Darling while the toast/email/webhook text stayed correct. HasValue is guaranteed - here — cpuExceeded requires it. */ + Darling while the toast/email/webhook text stayed correct. alertCpuValue.HasValue is + guaranteed here — the null arm above returns. */ await FireAsync(new AlertOutcome( key, serverName, "High CPU", $"{alertCpuValue:F0}% ({cpuMetricLabel})", @@ -410,12 +561,14 @@ await FireAsync(new AlertOutcome( ShortMessage: $"{cpuMetricLabel} at {alertCpuValue:F0}% (threshold: {_settings.CpuThresholdPercent}%)"), ct); } } - else if (_activeCpuAlert.TryGetValue(key, out var wasCpu) && wasCpu) /* :101 */ + else if (outcome == PersistenceOutcome.Resolve) /* :101 */ { - _activeCpuAlert[key] = false; /* :103 */ - /* :107 — resolve announced only while the alert is still enabled and unsuppressed - (disabling flips cpuExceeded false; neither means CPU actually recovered). */ - if (!suppressed && _settings.CpuEnabled) + /* The FALLING EDGE now comes from the gate rather than from a single sample dropping under the + bar, which is the other half of #3282: the pre-gate code resolved on the first clear sample, + so a 93%-for-two-minutes excursion produced a fire and a resolve 147 seconds apart and an + operator got both before either meant anything. The gate fires this exactly once, after + CpuClearSamples consecutive clears. Still gated on !suppressed, exactly as before. */ + if (!suppressed) { await NotifyResolutionAsync(new AlertResolution( key, serverName, "High CPU", @@ -425,6 +578,29 @@ await NotifyResolutionAsync(new AlertResolution( } } + /// + /// Persists one server's CPU gate record (#3282), absorbing store failures the way every other state + /// write in this class does: the gate has already decided this observation from the in-memory record, + /// so a dropped write costs the streak across a restart and never an alert. + /// + private async Task SaveCpuPersistenceAsync(string key, AlertPersistenceRecord record) + { + try + { + await _stateStore.SaveAlertPersistenceAsync(key, CpuPersistenceMetric, record); + } + catch (Exception ex) + { + /* NOT counted by #3013's counter, and Warning rather than Error — the same call + SaveOccurrencesAsync makes for the same reason. That counter is about READS the alert pass + performs and swallows, measured against a denominator of alert passes; a write in its + numerator would read as the pass going blind on a condition when in fact the condition was + evaluated correctly and only the memory of it was lost. The seed LOAD above is a read and is + counted there. */ + _logger?.LogWarning("Could not persist the CPU persistence gate for {ServerKey}: {Message}", key, ex.Message); + } + } + /* ---------------- blocking (Lite AlertEngine.cs:116-194) ---------------- */ private async Task CheckBlockingAsync( diff --git a/PerformanceMonitor.Alerting/AlertPersistenceRecord.cs b/PerformanceMonitor.Alerting/AlertPersistenceRecord.cs new file mode 100644 index 000000000..fe61eb132 --- /dev/null +++ b/PerformanceMonitor.Alerting/AlertPersistenceRecord.cs @@ -0,0 +1,40 @@ +/* + * 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; + +namespace PerformanceMonitor.Alerting; + +/// +/// One subject's persisted state, as the built-in alert catalog stores it +/// through (#3282). The gate itself is pure and owns no storage; this is the +/// row shape the caller writes between observations. +/// +/// Why the sample instant rides along. The gate counts OBSERVATIONS, and for a gauge read out of +/// the collected store an observation is one collected SAMPLE — not one alert sweep. The sweep runs every 30 +/// seconds while a CPU sample lands about once a minute, so a sweep that re-reads the row it read last time is +/// looking at the same observation twice. Counting sweeps would therefore satisfy "three consecutive breaches" +/// inside a 90-second excursion, which is shorter than the excursions #3282 measured and would leave the +/// defect in place while appearing to fix it. is what makes the count mean +/// samples: an observation whose sample instant has not advanced past it is not counted at all, and the streak +/// simply holds. +/// +/// A record struct so the whole answer moves as ONE value — a caller cannot advance the counters and +/// forget the sample instant, which would make every subsequent re-read look fresh. +/// +/// The gate's counters plus whether an incident is currently open. +/// +/// The sample instant of the newest observation already counted into , or null when +/// this subject has counted none yet (a fresh subject, or a host that supplies no sample instant — see +/// for what that degrades to). +/// +public readonly record struct AlertPersistenceRecord(PersistenceState State, DateTime? LastObservedSampleUtc) +{ + /// The record of a subject never observed: no streak, not firing, nothing counted. + public static AlertPersistenceRecord Initial => new(PersistenceState.Initial, null); +} diff --git a/PerformanceMonitor.Alerting/AlertServerSnapshot.cs b/PerformanceMonitor.Alerting/AlertServerSnapshot.cs index bba8295a5..d47e8d14d 100644 --- a/PerformanceMonitor.Alerting/AlertServerSnapshot.cs +++ b/PerformanceMonitor.Alerting/AlertServerSnapshot.cs @@ -6,6 +6,8 @@ * Licensed under the MIT License. See LICENSE file in the project root for full license information. */ +using System; + namespace PerformanceMonitor.Alerting; /// @@ -37,6 +39,18 @@ namespace PerformanceMonitor.Alerting; /// True for Azure SQL DB — skips the failed-jobs check (no SQL Agent), mirroring Lite's /// SqlEngineEdition != 5 call-site gate. /// +/// +/// The sample_time of the CPU reading / +/// came from, or null when the host has no sample instant for it. This is the CPU check's persistence-gate +/// observation identity (#3282), not display data: the gate counts consecutive breaching SAMPLES, and the +/// alert sweep is twice as fast as the samples arrive, so without it a re-read of one sample would count as +/// a second observation of the condition. +/// No default, so every host states it. A null degrades the gate to counting SWEEPS rather than +/// samples — weaker persistence, but the alert still fires, which is the correct direction for a monitoring +/// product where silence is indistinguishable from health. The opposite default (treat an unknown instant as +/// stale and never count it) would make the alert silent on a host that forgot to wire this, and nothing +/// would say so. +/// /// /// Suppression is an INPUT (Phase-5 review): true = evaluate-but-don't-deliver, exactly Lite's /// suppressPopups — edge-trigger watermarks don't advance where Lite's don't. Lite forwards @@ -49,4 +63,5 @@ public sealed record AlertServerSnapshot( double? SqlCpuPercent, double? TotalCpuPercent, bool IsAzureSqlDb, - bool Suppressed); + bool Suppressed, + DateTime? CpuSampleTimeUtc); diff --git a/PerformanceMonitor.Alerting/IAlertStateStore.cs b/PerformanceMonitor.Alerting/IAlertStateStore.cs index 13046e364..c379aec6e 100644 --- a/PerformanceMonitor.Alerting/IAlertStateStore.cs +++ b/PerformanceMonitor.Alerting/IAlertStateStore.cs @@ -125,4 +125,38 @@ Task> LoadIncidentOccurrenc /// Task SaveIncidentOccurrencesAsync( string serverKey, string metricName, IReadOnlyDictionary states); + + /// + /// Loads one subject's persisted state (#3282) — the built-in gauge + /// alerts' "how long has this condition held" counters, keyed the same (server, metric) way the + /// watermarks are. Returns null when nothing is persisted, which the caller reads as + /// . + /// + /// Persistence is the requirement, not an optimization, and it buys two different things. The + /// Firing flag is what stops a restart re-announcing an incident the operator already has open: + /// the pre-#3282 CPU check kept that flag in memory only, so the first post-restart sweep over a + /// standing condition delivered it again. The counters are what stop a restart mid-excursion throwing + /// away a streak that was about to fire. + /// + /// A host that cannot persist may return null and no-op the save. The degradation is + /// stated rather than hidden: the gate then lives for one process lifetime, so a restart resets the + /// streak and a sustained condition re-arms from zero — it fires N samples later, never "never". That + /// is strictly better than the pre-#3282 behaviour and is the intended fallback, not a broken state. + /// + Task LoadAlertPersistenceAsync(string serverKey, string metricName); + + /// + /// Upserts one subject's state (#3282). + /// + /// Unlike the watermark saves this is NOT on-change-only — it runs on every observation that + /// advances the gate, because a streak that is not persisted as it builds is a streak a restart can only + /// lose. That is still low frequency: one small upsert per server per new gauge sample (about one a + /// minute per server), and the caller skips the write entirely when the record is unchanged, which is + /// what the value-equality of is for. + /// + /// Implementations absorb their own failures like the watermark writes. A dropped save costs the + /// streak on a restart, never a missed or duplicated alert: the gate has already decided this + /// observation's outcome from the in-memory record by the time this is called. + /// + Task SaveAlertPersistenceAsync(string serverKey, string metricName, AlertPersistenceRecord record); } diff --git a/README.md b/README.md index f2ff883c2..34b8ccebb 100644 --- a/README.md +++ b/README.md @@ -256,7 +256,7 @@ Every edition includes a real-time alert engine that monitors for performance is | **Long-running queries** | 5 minutes | Fires when any query exceeds the elapsed-time threshold | | **TempDB space** | 80% | Fires when TempDB usage exceeds the percentage threshold. Measured against tempdb's **growth ceiling** (`SUM(max_size)` over the ROWS files) where there is one, and against the current allocation where the files grow without limit — so the percentage means "distance to the point where tempdb cannot grow further" on every engine | | **Long-running agent jobs** | 3× average | Fires when a job's current duration exceeds a multiple of its historical average | -| **High CPU** | 80% | Fires when total CPU (SQL + other) exceeds the threshold | +| **High CPU** | 80%, held for 3 samples | Fires when total CPU (SQL + other) is at or above the threshold on **3 consecutive collected samples** — about three minutes at the one-minute CPU sample cadence — and resolves after 2 consecutive samples below it. A momentary spike above a steady baseline is detected but is not an incident, so it is not delivered. The count is per SAMPLE, not per alert sweep, so a sweep re-reading a sample it has already seen does not advance it. A CPU reading that stops arriving holds the count where it is rather than announcing a recovery | | **Volume free space** | 10% or 5 GB free | Fires when a monitored volume's free space drops below the percentage or absolute threshold (either check can be disabled). Never fires on Azure SQL Database. | | **Failed agent job** | 60-minute lookback | Fires when a SQL Agent job run fails within the lookback window. Skipped on Azure SQL Database. | | **Server unreachable** | N/A | Fires when a monitored server goes offline or comes back online |