diff --git a/Darling/Darling.Tests/AlertStoredValueTests.cs b/Darling/Darling.Tests/AlertStoredValueTests.cs index 4aaa39f8b1..d43b0754e0 100644 --- a/Darling/Darling.Tests/AlertStoredValueTests.cs +++ b/Darling/Darling.Tests/AlertStoredValueTests.cs @@ -135,7 +135,7 @@ public Task RecordAlertAsync(AlertHistoryRecord record) public Task GetLastWebhookSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => Task.FromResult(null); - public Task GetLastAlertTimeAsync(string serverId, string metricName) => + public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => Task.FromResult(null); } diff --git a/Darling/Darling.Tests/DarlingAlertingTests.cs b/Darling/Darling.Tests/DarlingAlertingTests.cs index 60fe73ff31..b3e546df43 100644 --- a/Darling/Darling.Tests/DarlingAlertingTests.cs +++ b/Darling/Darling.Tests/DarlingAlertingTests.cs @@ -336,6 +336,86 @@ await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, async (cleanup } } + /// + /// #2716: the primitive Darling's Postgres Tier-0-predictor cooldown (wraparound/xmin/replication + /// slot — DarlingWorker's _lastPostgresAlert, keyed per server|metric|subject) restart-seeds + /// itself from. Those three alerts have no rolling-COUNT watermark to persist through + /// — they are a plain last-alerted TIME per subject — so the fix + /// reuses 's dedupKey filter (already proven + /// for the #1154 email/webhook cooldowns) instead of adding new schema. This proves the filter + /// actually isolates by subject rather than degenerating to the metric-level MAX: two subjects + /// alerted at different times on the SAME (server, metric) must each seed from their OWN row, not + /// the newer one's. + /// + [Fact] + public async Task PgAlertHistoryStore_GetLastAlertTimeAsync_FiltersByDedupKey_ForRestartSeeding() + { + var connectionString = Environment.GetEnvironmentVariable("DARLING_TEST_PG"); + Assert.SkipWhen(string.IsNullOrEmpty(connectionString), + "Set DARLING_TEST_PG to a Postgres connection string to run the live alert-history test."); + + var ct = TestContext.Current.CancellationToken; + + using var connection = new NpgsqlConnection(connectionString); + await connection.OpenAsync(ct); + await PgMigrations.MigrateAsync(connection, ct); + await DeleteTestRowsAsync(connection, ct); + + await using var postgres = NpgsqlDataSource.Create(connectionString!); + var bodySucceeded = false; + try + { + var historyStore = new PgAlertHistoryStore(postgres); + const string metricName = "Wraparound Risk"; + + /* Real serializer, not hand-built JSON — the same path AlertOutcome.Context takes before + RecordAlertAsync ever sees it (AlertContextBuilders/AlertContextSerializer.Serialize). */ + string ContextFor(string subject) => + AlertContextSerializer.Serialize(new AlertContext + { + Incidents = new List { new(subject, new[] { subject }) }, + }); + + /* "walnut" alerted first (older), "cashew" alerted second (newer) — same server, same + metric, different subjects. An unfiltered MAX would report cashew's time for BOTH. */ + await historyStore.RecordAlertAsync(new AlertHistoryRecord( + TestServerKey, TestServerName, metricName, + "92%", "90%", 92, 90, + AlertSent: true, NotificationType: "tray", SendError: null, + Muted: false, DetailText: null, ContextJson: ContextFor("walnut"))); + + await Task.Delay(TimeSpan.FromMilliseconds(50), ct); /* force a distinguishable alert_time */ + + await historyStore.RecordAlertAsync(new AlertHistoryRecord( + TestServerKey, TestServerName, metricName, + "95%", "90%", 95, 90, + AlertSent: true, NotificationType: "tray", SendError: null, + Muted: false, DetailText: null, ContextJson: ContextFor("cashew"))); + + var walnutSeed = await historyStore.GetLastAlertTimeAsync(TestServerKey, metricName, dedupKey: "walnut"); + var cashewSeed = await historyStore.GetLastAlertTimeAsync(TestServerKey, metricName, dedupKey: "cashew"); + var unfiltered = await historyStore.GetLastAlertTimeAsync(TestServerKey, metricName); + var missingSubjectSeed = await historyStore.GetLastAlertTimeAsync(TestServerKey, metricName, dedupKey: "pistachio"); + + Assert.NotNull(walnutSeed); + Assert.NotNull(cashewSeed); + /* The whole point of the fix: walnut's seed must be its OWN (older) time, not cashew's. */ + Assert.True(walnutSeed < cashewSeed, + $"walnut ({walnutSeed:O}) must seed from its own row, strictly before cashew's ({cashewSeed:O})"); + /* Unfiltered stays the pre-#2716 behavior: the newest row across all subjects. */ + Assert.Equal(cashewSeed, unfiltered); + /* A subject with no history at all seeds nothing — a restart must not invent a cooldown. */ + Assert.Null(missingSubjectSeed); + + bodySucceeded = true; + } + finally + { + await LiveStoreCleanup.RunAsync(connectionString!, bodySucceeded, async (cleanup, cleanupCt) => + await DeleteTestRowsAsync(cleanup, cleanupCt)); + } + } + [Fact] public async Task EndToEnd_PgMuteRuleStore_CrudRoundTrip() { diff --git a/Darling/Darling.Tests/DarlingDeliveryModeTests.cs b/Darling/Darling.Tests/DarlingDeliveryModeTests.cs index 25243c7f28..6ef66abfd8 100644 --- a/Darling/Darling.Tests/DarlingDeliveryModeTests.cs +++ b/Darling/Darling.Tests/DarlingDeliveryModeTests.cs @@ -297,7 +297,7 @@ public Task RecordAlertAsync(AlertHistoryRecord record) public Task GetLastWebhookSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => Task.FromResult(null); - public Task GetLastAlertTimeAsync(string serverId, string metricName) => + public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => Task.FromResult(null); } } diff --git a/Darling/Darling.Tests/DarlingSelfAlertTests.cs b/Darling/Darling.Tests/DarlingSelfAlertTests.cs index 480828c501..c418caed44 100644 --- a/Darling/Darling.Tests/DarlingSelfAlertTests.cs +++ b/Darling/Darling.Tests/DarlingSelfAlertTests.cs @@ -120,7 +120,7 @@ public Task RecordAlertAsync(AlertHistoryRecord record) public Task GetLastWebhookSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => Task.FromResult(null); - public Task GetLastAlertTimeAsync(string serverId, string metricName) => + public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => Task.FromResult(null); } diff --git a/Darling/Darling.Tests/StoreSelfMetricsTests.cs b/Darling/Darling.Tests/StoreSelfMetricsTests.cs index 618e788353..90683d6b59 100644 --- a/Darling/Darling.Tests/StoreSelfMetricsTests.cs +++ b/Darling/Darling.Tests/StoreSelfMetricsTests.cs @@ -714,7 +714,7 @@ private sealed class CadenceFakeHistoryStore : IAlertHistoryStore Task.FromResult(null); public Task GetLastWebhookSentUtcAsync(string serverId, string metricName, string? dedupKey = null) => Task.FromResult(null); - public Task GetLastAlertTimeAsync(string serverId, string metricName) => + public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => Task.FromResult(null); } diff --git a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs index d1f9eb7cc9..fee71439a1 100644 --- a/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs +++ b/Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs @@ -458,6 +458,25 @@ already living in PerformanceMonitor.Alerting - reusing it here is what keeps th private readonly ConcurrentDictionary _activePgBlockingAlert = new(StringComparer.Ordinal); private readonly ConcurrentDictionary _lastAlertedPgBlockingCount = 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 + IAlertStateStore on each server's first post-restart sweep (EnsureWatermarksSeededAsync), but + nothing does the equivalent here, so a restart resets the watermark to 0 and the very next sweep + re-fires on a deadlock/blocking count still sitting in the rolling window from before the + restart. Seeded once per (server, metric) key, mirroring AlertEngine's _seededServerKeys. */ + private readonly ConcurrentDictionary _pgDeadlockWatermarkSeeded = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary _pgBlockingWatermarkSeeded = new(StringComparer.Ordinal); + + /* #2716: the three Tier 0 predictors' cooldown (_lastPostgresAlert, keyed per server|metric|subject) + has the same restart gap, but no watermark COUNT to seed — it is a plain last-alerted TIME per + subject, so IAlertStateStore's int-watermark shape does not fit it. Seeded instead from + IAlertHistoryStore.GetLastAlertTimeAsync's existing #1154 dedup-key filter (finding.Subject IS + the #1140 dedup fingerprint these alerts already fire with), which already reconstructs a + per-fingerprint last-alerted time for the email/webhook cooldowns and needs no new schema. + Guards one history read per (server, metric, subject) for the life of the process — see the call + site for why an unconditional per-sweep read would be a real cost, not just noise. */ + private readonly ConcurrentDictionary _postgresAlertHistorySeeded = new(StringComparer.Ordinal); + /// /// Held for the same reason is: the Postgres Deadlocks/Blocking alerts /// (#2711) need to write a resolution history row on the active→inactive transition, exactly like @@ -2829,6 +2848,27 @@ await adapter.GetXminHorizonAsync(runtime.ServerId, cancellationToken), CultureInfo.InvariantCulture, $"{snapshot.ServerKey}|{finding.MetricName}|{finding.Subject}"); + /* #2716: this cooldown has no in-memory entry for a subject this process has never + evaluated, which includes every subject after a restart even if it was alerted on + moments before. Seed it ONCE per key from history before trusting its absence — + finding.Subject IS the #1140 dedup fingerprint this alert already fires with, so + GetLastAlertTimeAsync's existing #1154 filter reconstructs exactly the per-subject + time this cooldown needs, no new schema required. Guarded by + _postgresAlertHistorySeeded so a subject that has never alerted (the common case — + most databases never cross the wraparound line) costs one history read per process + lifetime, not one per sweep forever. */ + if (!_lastPostgresAlert.ContainsKey(cooldownKey) + && _historyStore is not null + && _postgresAlertHistorySeeded.TryAdd(cooldownKey, true)) + { + var seeded = await _historyStore.GetLastAlertTimeAsync( + snapshot.ServerKey, finding.MetricName, dedupKey: finding.Subject); + if (seeded.HasValue) + { + _lastPostgresAlert[cooldownKey] = seeded.Value; + } + } + if (_lastPostgresAlert.TryGetValue(cooldownKey, out var last) && now - last < cooldown) { continue; @@ -2914,9 +2954,24 @@ private async Task EvaluatePgDeadlocksAsync( const string metricName = "Deadlocks Detected"; var key = snapshot.ServerKey; + var stateStore = new PgAlertStateStore(_postgres, _logger); try { + /* #2716: seed the watermark from the same config_edge_trigger_watermarks row + AlertEngine's own SQL Server "Deadlocks Detected" twin reads/writes — the parity metric + name #2711 deliberately chose means no new column or row shape is needed, only a read + before trusting an in-memory zero. Once per key, mirroring AlertEngine's + EnsureWatermarksSeededAsync/_seededServerKeys. */ + if (_pgDeadlockWatermarkSeeded.TryAdd(key, true)) + { + var seeded = await stateStore.LoadEdgeTriggerWatermarkAsync(key, metricName); + if (seeded.HasValue) + { + _lastAlertedPgDeadlockCount[key] = seeded.Value; + } + } + var now = DateTime.UtcNow; var windowStart = now.AddHours(-AlertEngine.RollingCountWindowHours); @@ -2934,6 +2989,14 @@ collection_time would put a report in the wrong window and move it every cycle. var decision = RollingCountAlertGate.Evaluate( count, PgDeadlockCountThreshold, watermark, cooldownElapsed, suppressed: false); _lastAlertedPgDeadlockCount[key] = decision.Watermark; + if (decision.Watermark != watermark) + { + /* On-change only (#1145's own contract) — persist AFTER the in-memory update so a + store failure never desyncs the two; the in-memory watermark still gates this + process even if the write is lost, same posture as every other watermark save + in this codebase. */ + await stateStore.SaveEdgeTriggerWatermarkAsync(key, metricName, decision.Watermark); + } var wasActive = _activePgDeadlockAlert.TryGetValue(key, out var activeBefore) && activeBefore; _activePgDeadlockAlert[key] = decision.Active; @@ -3002,9 +3065,20 @@ private async Task EvaluatePgBlockingAsync( const string metricName = "Blocking Detected"; var key = snapshot.ServerKey; + var stateStore = new PgAlertStateStore(_postgres, _logger); try { + /* #2716: same restart-survival seed as EvaluatePgDeadlocksAsync — see its comment. */ + if (_pgBlockingWatermarkSeeded.TryAdd(key, true)) + { + var seeded = await stateStore.LoadEdgeTriggerWatermarkAsync(key, metricName); + if (seeded.HasValue) + { + _lastAlertedPgBlockingCount[key] = seeded.Value; + } + } + var now = DateTime.UtcNow; var windowStart = now.AddHours(-AlertEngine.RollingCountWindowHours); @@ -3021,6 +3095,10 @@ private async Task EvaluatePgBlockingAsync( var decision = RollingCountAlertGate.Evaluate( count, PgBlockingCountThreshold, watermark, cooldownElapsed, suppressed: false); _lastAlertedPgBlockingCount[key] = decision.Watermark; + if (decision.Watermark != watermark) + { + await stateStore.SaveEdgeTriggerWatermarkAsync(key, metricName, decision.Watermark); + } var wasActive = _activePgBlockingAlert.TryGetValue(key, out var activeBefore) && activeBefore; _activePgBlockingAlert[key] = decision.Active; diff --git a/Darling/PerformanceMonitor.Darling.Service/PgAlertHistoryStore.cs b/Darling/PerformanceMonitor.Darling.Service/PgAlertHistoryStore.cs index 94ef8fdc06..5c9b2ffdf9 100644 --- a/Darling/PerformanceMonitor.Darling.Service/PgAlertHistoryStore.cs +++ b/Darling/PerformanceMonitor.Darling.Service/PgAlertHistoryStore.cs @@ -112,9 +112,10 @@ AND notification_type IN ('email', 'email+webhook') ReadMaxAlertTimeAsync(serverId, metricName, dedupKey, @" AND notification_type IN ('webhook', 'email+webhook')", "WebhookAlert"); - public Task GetLastAlertTimeAsync(string serverId, string metricName) => - /* Unfiltered — any channel/result (the analysis cooldown is stamped unconditionally). */ - ReadMaxAlertTimeAsync(serverId, metricName, dedupKey: null, extraFilter: "", logScope: "AnalysisNotify"); + public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => + /* Unfiltered — any channel/result (the analysis cooldown is stamped unconditionally). #2716 + reuses the dedupKey filter to seed Postgres Tier-0-predictor cooldowns across a restart. */ + ReadMaxAlertTimeAsync(serverId, metricName, dedupKey, extraFilter: "", logScope: "AnalysisNotify"); private async Task ReadMaxAlertTimeAsync( string serverId, string metricName, string? dedupKey, string extraFilter, string logScope) @@ -129,14 +130,17 @@ FROM config_alert_log WHERE server_id = $1 AND metric_name = $2" + extraFilter /* #1154: the per-fingerprint filter — Lite's anchored LIKE on the serialized - "DedupKey":"" property (the hex key has no LIKE wildcards; NULL - context_json rows fail the match). */ - + (dedupKey is null ? "" : "\nAND context_json LIKE $3"), connection); + "DedupKey":"" property. #2716 added a caller whose dedupKey is a raw, + human-readable name rather than a hash, so the pattern is built (and any LIKE/JSON + special characters escaped) by AlertContextSerializer.BuildDedupKeyLikePattern + rather than hand-concatenated here — see its doc comment for why. NULL context_json + rows fail the match either way. */ + + (dedupKey is null ? "" : "\nAND context_json LIKE $3 ESCAPE '\\'"), connection); command.Parameters.AddWithValue(sid); command.Parameters.AddWithValue(metricName); if (dedupKey is not null) { - command.Parameters.AddWithValue("%\"DedupKey\":\"" + dedupKey + "\"%"); + command.Parameters.AddWithValue(AlertContextSerializer.BuildDedupKeyLikePattern(dedupKey)); } var result = await command.ExecuteScalarAsync(); diff --git a/Lite.Tests/StoreRoundTripTests.cs b/Lite.Tests/StoreRoundTripTests.cs index 8e7d6bfa38..902b80ede7 100644 --- a/Lite.Tests/StoreRoundTripTests.cs +++ b/Lite.Tests/StoreRoundTripTests.cs @@ -212,6 +212,40 @@ any dedupKey filter must exclude (it would NRE/over-match a naive scan). */ Assert.Null(await store.GetLastWebhookSentUtcAsync("9", "Blocking Detected", "eeee5555")); } + /// + /// #2716's Postgres Tier-0-predictor cooldown seed passes a RAW database/slot name as dedupKey — + /// not a #1140 hash — so this filter has to be correct for arbitrary text, not just hex. Three + /// failure modes, proven against the real store rather than the escaping logic in isolation: + /// a quote/backslash breaks the naive string-concatenation match (the value Serialize() actually + /// writes is JSON-escaped, the old hand-built search pattern was not); an underscore is a SQL LIKE + /// wildcard, so a naive pattern for "orders_db" would also match "ordersXdb" for any X; and a + /// non-ASCII character is escaped by the default JSON encoder into a \uXXXX sequence the old + /// pattern never accounted for either. + /// + [Fact] + public async Task GetLastAlertTime_WithDedupKey_HandlesRawNonHexSubjectsSafely() + { + var store = new DuckDbAlertHistoryStore(_duckDb); + + // Quote/backslash: a subject like a Windows-style path or a quoted identifier. + await RecordWithContextAsync(store, "20", "Wraparound Risk", "tray", JsonWith("db\"with\\quote")); + Assert.NotNull(await store.GetLastAlertTimeAsync("20", "Wraparound Risk", "db\"with\\quote")); + + // Underscore: must match only the exact subject, never a same-shaped different one. + await RecordWithContextAsync(store, "21", "Wraparound Risk", "tray", JsonWith("orders_db")); + Assert.NotNull(await store.GetLastAlertTimeAsync("21", "Wraparound Risk", "orders_db")); + Assert.Null(await store.GetLastAlertTimeAsync("21", "Wraparound Risk", "ordersXdb")); + + // Non-ASCII: the default JSON encoder escapes this to a \uXXXX sequence before it is stored. + await RecordWithContextAsync(store, "22", "Wraparound Risk", "tray", JsonWith("café")); + Assert.NotNull(await store.GetLastAlertTimeAsync("22", "Wraparound Risk", "café")); + + // Percent: the other SQL LIKE wildcard, same shape as the underscore case. + await RecordWithContextAsync(store, "23", "Wraparound Risk", "tray", JsonWith("100%done")); + Assert.NotNull(await store.GetLastAlertTimeAsync("23", "Wraparound Risk", "100%done")); + Assert.Null(await store.GetLastAlertTimeAsync("23", "Wraparound Risk", "100Xdone")); + } + [Fact] public async Task EdgeTriggerWatermark_SaveLoad_RoundTripsAndUpserts() { diff --git a/Lite.Tests/WebhookCooldownSeedTests.cs b/Lite.Tests/WebhookCooldownSeedTests.cs index 91a20d9178..3c78967cc9 100644 --- a/Lite.Tests/WebhookCooldownSeedTests.cs +++ b/Lite.Tests/WebhookCooldownSeedTests.cs @@ -120,7 +120,7 @@ private sealed class FakeHistoryStore : IAlertHistoryStore return Task.FromResult(null); return Task.FromResult(LastWebhookSent); } - public Task GetLastAlertTimeAsync(string serverId, string metricName) => Task.FromResult(null); + public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) => Task.FromResult(null); } private static AlertContext ContextWith(string dedupKey) => new() diff --git a/Lite/Services/DuckDbAlertHistoryStore.cs b/Lite/Services/DuckDbAlertHistoryStore.cs index a52ffa504c..caf6c463f2 100644 --- a/Lite/Services/DuckDbAlertHistoryStore.cs +++ b/Lite/Services/DuckDbAlertHistoryStore.cs @@ -150,9 +150,12 @@ INSERT INTO config_alert_log (alert_time, server_id, server_name, metric_name, c /* A successful email send is logged with a notification_type of 'email' / 'email+webhook' and a null send_error — that mirrors exactly when the cooldown is stamped after SendEmailAsync. - #1154: when a dedupKey is supplied, push the per-fingerprint filter into - DuckDB via an anchored LIKE on the serialized "DedupKey":"" property - (the hex key has no LIKE wildcards; NULL context_json rows fail the match). */ + #1154: when a dedupKey is supplied, push the per-fingerprint filter into DuckDB via + an anchored LIKE on the serialized "DedupKey":"" property, built (with any + LIKE/JSON special characters escaped) by AlertContextSerializer.BuildDedupKeyLikePattern + rather than hand-concatenated — see its doc comment for why a hand-built pattern is + unsafe for a caller whose dedupKey is not a hash. NULL context_json rows fail the + match either way. */ command.CommandText = @" SELECT MAX(alert_time) FROM config_alert_log @@ -160,11 +163,11 @@ FROM config_alert_log AND metric_name = $2 AND notification_type IN ('email', 'email+webhook') AND send_error IS NULL" - + (dedupKey is null ? "" : "\nAND context_json LIKE $3"); + + (dedupKey is null ? "" : "\nAND context_json LIKE $3 ESCAPE '\\'"); command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = sid }); command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = metricName }); if (dedupKey is not null) - command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = "%\"DedupKey\":\"" + dedupKey + "\"%" }); + command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = AlertContextSerializer.BuildDedupKeyLikePattern(dedupKey) }); var result = await command.ExecuteScalarAsync(); if (result == null || result == DBNull.Value) return null; @@ -213,19 +216,22 @@ is explicit (the cooldown subtraction is tick math regardless). */ when WebhookSent is true, so the type alone implies success. send_error tracks the EMAIL channel, so it is NOT filtered on: an email-failed-but-webhook-sent row must still seed the cooldown. - #1154: when a dedupKey is supplied, push the per-fingerprint filter into - DuckDB via an anchored LIKE on the serialized "DedupKey":"" property. */ + #1154: when a dedupKey is supplied, push the per-fingerprint filter into DuckDB via + an anchored LIKE built (with any LIKE/JSON special characters escaped) by + AlertContextSerializer.BuildDedupKeyLikePattern — see its doc comment for why a + hand-built "%\"DedupKey\":\"\"%" pattern is unsafe for a caller whose dedupKey + is not a hash. */ command.CommandText = @" SELECT MAX(alert_time) FROM config_alert_log WHERE server_id = $1 AND metric_name = $2 AND notification_type IN ('webhook', 'email+webhook')" - + (dedupKey is null ? "" : "\nAND context_json LIKE $3"); + + (dedupKey is null ? "" : "\nAND context_json LIKE $3 ESCAPE '\\'"); command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = sid }); command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = metricName }); if (dedupKey is not null) - command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = "%\"DedupKey\":\"" + dedupKey + "\"%" }); + command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = AlertContextSerializer.BuildDedupKeyLikePattern(dedupKey) }); var result = await command.ExecuteScalarAsync(); if (result == null || result == DBNull.Value) return null; @@ -249,8 +255,13 @@ is explicit (the cooldown subtraction is tick math regardless). */ /// email cooldown (which filters to successful sends), the analysis /// cooldown is stamped unconditionally, so the persisted equivalent /// is the latest row for that metric_name, period. + /// + /// #2716: when is supplied, the same #1154 anchored-LIKE filter its + /// email/webhook siblings use is applied here too, reconstructing a per-fingerprint last-alerted + /// time rather than a metric-level one. + /// /// - public async Task GetLastAlertTimeAsync(string serverId, string metricName) + public async Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) { var sid = int.TryParse(serverId, out var s) ? s : 0; try @@ -278,9 +289,12 @@ between two findings could seed one from the other's history. SELECT MAX(alert_time) FROM config_alert_log WHERE server_id = $1 -AND metric_name = $2"; +AND metric_name = $2" + + (dedupKey is null ? "" : "\nAND context_json LIKE $3 ESCAPE '\\'"); command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = sid }); command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = metricName }); + if (dedupKey is not null) + command.Parameters.Add(new DuckDB.NET.Data.DuckDBParameter { Value = AlertContextSerializer.BuildDedupKeyLikePattern(dedupKey) }); var result = await command.ExecuteScalarAsync(); if (result == null || result == DBNull.Value) return null; diff --git a/PerformanceMonitor.Notifications/AlertContext.cs b/PerformanceMonitor.Notifications/AlertContext.cs index fe9d72261e..f66754b433 100644 --- a/PerformanceMonitor.Notifications/AlertContext.cs +++ b/PerformanceMonitor.Notifications/AlertContext.cs @@ -327,18 +327,64 @@ public static class AlertContextSerializer /// /// True when the persisted carries the given #1140 dedup fingerprint /// (#1154 per-fingerprint cooldown seed). Anchored substring match on the serialized - /// "DedupKey":"<hex>" property — safe because the key is lowercase SHA-256 hex (no JSON - /// escaping, no collision with any other serialized field) and emits PascalCase - /// with default options. Returns false on null/blank input — the Dashboard scan visits many rows whose - /// ContextJson is null (tray/muted/server-reachability rows), and an un-guarded match would NRE. - /// Centralizes the JSON shape so the Dashboard store cannot drift; the Lite store re-states the same - /// anchor in its SQL LIKE for push-down and is guarded by a store round-trip test. + /// "DedupKey":"<value>" property. Originally documented as safe only for lowercase + /// SHA-256 hex fingerprints (no JSON escaping, no collision with any other serialized field) — #2716 + /// added a caller (Postgres Tier-0-predictor cooldown seeding) that passes a raw, human-readable + /// database/slot name instead of a hash, which a bare string-concatenation match gets wrong for any + /// name containing a quote, backslash, or non-ASCII character (the value + /// actually emits is JSON-escaped; the naively-built search pattern was not). Fixed at the root via + /// rather than by keeping this hex-only and pushing an + /// escaping obligation onto every caller — so it is now correct for ANY dedup key, hashed or not. + /// Returns false on null/blank input — the Dashboard scan visits many rows whose ContextJson is + /// null (tray/muted/server-reachability rows), and an un-guarded match would NRE. Centralizes the JSON + /// shape so the Dashboard store cannot drift; the Lite/Darling stores re-state the same anchor in + /// their SQL LIKE via for push-down and are guarded by a + /// store round-trip test. /// public static bool ContextJsonContainsDedupKey(string? contextJson, string? dedupKey) { if (string.IsNullOrEmpty(contextJson) || string.IsNullOrEmpty(dedupKey)) return false; - return contextJson.Contains("\"DedupKey\":\"" + dedupKey + "\"", StringComparison.Ordinal); + return contextJson.Contains(BuildDedupKeyJsonFragment(dedupKey), StringComparison.Ordinal); + } + + /// + /// The exact JSON fragment would produce for this dedup key's + /// "DedupKey":"..." property — computed by round-tripping through + /// + /// itself (default options, the same ones uses) rather than hand-rolling the + /// escaping rules, so this can never drift from what actually gets written. A hex fingerprint + /// round-trips unchanged; a raw name with a quote, backslash, or non-ASCII character comes back + /// correctly escaped the same way the persisted value was. + /// + public static string BuildDedupKeyJsonFragment(string dedupKey) + { + var quoted = JsonSerializer.Serialize(dedupKey); + // JsonSerializer.Serialize(string) always returns a quoted JSON string literal (e.g. "café"), + // so stripping exactly one leading and one trailing quote yields the escaped inner content. + var escaped = quoted.Substring(1, quoted.Length - 2); + return "\"DedupKey\":\"" + escaped + "\""; + } + + /// + /// The full LIKE pattern (with % wildcards) a SQL store should use to find + /// inside a persisted ContextJson column — pair with + /// ESCAPE '\' on the query. Starts from the same JSON-escaped fragment + /// computes, then additionally escapes the three characters + /// LIKE itself treats specially (\, %, _) so a database/slot name + /// containing an underscore (e.g. orders_db) matches only itself instead of also matching + /// ordersXdb for any character X. Escaping the backslash FIRST is load-bearing: it must run + /// before the %/_ escaping so the backslashes those two insert are not themselves + /// re-escaped. + /// + public static string BuildDedupKeyLikePattern(string dedupKey) + { + var fragment = BuildDedupKeyJsonFragment(dedupKey); + var likeSafe = fragment + .Replace("\\", "\\\\", StringComparison.Ordinal) + .Replace("%", "\\%", StringComparison.Ordinal) + .Replace("_", "\\_", StringComparison.Ordinal); + return "%" + likeSafe + "%"; } public static string Serialize(AlertContext context) diff --git a/PerformanceMonitor.Notifications/IAlertHistoryStore.cs b/PerformanceMonitor.Notifications/IAlertHistoryStore.cs index 2fd6485e01..07f78b4944 100644 --- a/PerformanceMonitor.Notifications/IAlertHistoryStore.cs +++ b/PerformanceMonitor.Notifications/IAlertHistoryStore.cs @@ -65,8 +65,14 @@ public interface IAlertHistoryStore /// /// MAX(alert_time) UNFILTERED (any channel/result) — seeds the analysis /// per-finding cooldown across restart. Stamped unconditionally upstream. + /// + /// When is non-null (#1154 per-fingerprint cooldown, reused by #2716 to + /// seed Darling's Postgres Tier-0-predictor cooldowns), the result is additionally restricted to rows + /// whose persisted ContextJson carries that #1140 dedup fingerprint. Null = the metric-level + /// seed (the pre-#1154 behavior, used by the non-fingerprinted fallback). + /// /// - Task GetLastAlertTimeAsync(string serverId, string metricName); + Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null); } /// diff --git a/deprecated/Dashboard/Services/JsonAlertHistoryStore.cs b/deprecated/Dashboard/Services/JsonAlertHistoryStore.cs index 31843f0cdd..85035b87e9 100644 --- a/deprecated/Dashboard/Services/JsonAlertHistoryStore.cs +++ b/deprecated/Dashboard/Services/JsonAlertHistoryStore.cs @@ -170,7 +170,12 @@ public Task RecordAlertAsync(AlertHistoryRecord record) /// NotificationType (which can be "email", "webhook", or "tray" on /// Dashboard) and SendError. Returns null if no matching entry. /// - public Task GetLastAlertTimeAsync(string serverId, string metricName) + /// + /// When is non-null (#1154, reused by #2716), the scan is + /// additionally restricted to rows whose ContextJson carries that #1140 fingerprint, same as + /// this store's email/webhook siblings. + /// + public Task GetLastAlertTimeAsync(string serverId, string metricName, string? dedupKey = null) { lock (_alertLogLock) { @@ -179,6 +184,7 @@ public Task RecordAlertAsync(AlertHistoryRecord record) { if (entry.ServerId != serverId) continue; if (entry.MetricName != metricName) continue; + if (dedupKey is not null && !AlertContextSerializer.ContextJsonContainsDedupKey(entry.ContextJson, dedupKey)) continue; if (max == null || entry.AlertTime > max.Value) max = entry.AlertTime; } return Task.FromResult(max);