From bd5efc21fd34bb4613d7c494aff7a76158cf33a2 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 16 Oct 2026 15:39:55 -0500 Subject: [PATCH 1/5] fix(notifications): a resolve no longer marks the alert's bell entry unread A RESOLVED event reset the alert's bell recipient row to unread, so good news re-bumped the bell. The in-app channel now updates the entry in place (title "Resolved: ...") and keeps the read state. A subscription whose events name RESOLVED and differ from the default set keeps the old behavior. A reopen suppressed as a duplicate refreshes the entry's text so it never keeps reading "Resolved". External channels are unchanged. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + docs/NOTIFICATIONS.md | 2 +- .../qbits/alerts/AlertsPreferences.java | 3 +- .../qbits/alerts/engine/AlertRouter.java | 79 ++++++- .../metadata/AlertsTableMetaDataHelper.java | 3 +- .../notifications/NotificationService.java | 36 ++- .../alerts/processes/AlertsActionSteps.java | 3 +- src/main/resources/help/qbit-alerts.yaml | 1 + .../alerts/engine/QuietNotificationsTest.java | 222 ++++++++++++++++++ 9 files changed, 337 insertions(+), 13 deletions(-) create mode 100644 src/test/java/com/kingsrook/qbits/alerts/engine/QuietNotificationsTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 0620f12..b387250 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to this project are documented here. The format follows Keep ## [0.1.0] - Unreleased ### Fixed +- Bell noise: a resolve marked the alert's bell entry unread again. It now updates the entry in place (title "Resolved: ...") and leaves the read state alone. A subscription whose events name RESOLVED and differ from the default set (`AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS`) keeps the old behavior. External channels are unchanged. - Under OAuth2, per-recipient checks (alert visibility for external channels, follower record access, audience sessions) never worked: they used token-less automated sessions, which OAuth2 rejects, so external sends fell back to the conservative rule. When the host declares the QQQ `alerts` service principal (`AlertsQBitConfig.withServicePrincipal`, default `alerts`), these checks run on the recipient's behalf through it: the recipient's current access intersected with the principal's. Alert recording and routing bookkeeping stay on the system session, so alerting does not depend on the identity provider being up. With no principal declared, behavior is unchanged. Needs the QQQ release with service identities (qqq#1255, #1256). - Audit noise: every monitor run (lease claim, then run result) and every reminder check wrote a "Record was Edited" audit (about 7,000 `alerts_monitor` and 1,800 `alerts_alert` audits in 6 hours on one production host, most with no field details). Engine updates that touch only bookkeeping fields (`RuntimeFields`: monitor `claimedBy`, `claimedUntil`, `lastRunAt`, `nextRunAt`, `lastStatus`, `lastValue`, `lastError`, `lastElapsedMs`, `consecutiveFailures`, `conditionSince`; alert `timedNotificationCheckedAt`, `lastNotifiedAt`, `recordRefCount`) are written with `omitDmlAudit`. Configuration edits, raises and alert state changes are audited as before, as are UI and API edits of any field. - Read the QBit metadata version from the Maven-filtered artifact version so release builds report their release version. diff --git a/docs/NOTIFICATIONS.md b/docs/NOTIFICATIONS.md index 872e384..7e29107 100644 --- a/docs/NOTIFICATIONS.md +++ b/docs/NOTIFICATIONS.md @@ -233,7 +233,7 @@ QAlerts.notify(new NotificationSignal("Your export failed") | Source | How | Category | |---|---|---| | Failed or partial jobs | The qbit's process lifecycle listener: a run that fails, or completes with error summary lines, notifies the user who started it (the session user; system runs are skipped). The link is the run's table, or `jobRunLinkPattern` with `{processName}` and `{processUUID}`. Turn it off with `notifyFailedJobs(false)`. | JOB | -| Alerts they subscribe to | The router's in-app channel writes an ALERT notification per alert (sourceKey `alert:`) and a recipient row per routed user. A repeat or reopen marks it unread again. | ALERT | +| Alerts they subscribe to | The router's in-app channel writes an ALERT notification per alert (sourceKey `alert:`) and a recipient row per routed user. A raise, reopen, severity increase, reminder or escalation marks it unread again. A resolve only updates it in place (title "Resolved: ...", read state unchanged), unless the subscription chose resolve notices on purpose (its events name RESOLVED and differ from the default set). | ALERT | | Shares, record locks, approvals | Producers call `QAlerts.notify(... user(id))`. The first wiring is the SQL Workbench notebook share in kof22. | SHARE | | Security notices | `AlertsSecurityNotices` (new sign-in, API key created, API key expiring, permission change). Core has no authentication events today, so hosts call it from their authentication customizer. A core hook is follow-up work (section 12, Q5). Security is never mutable in-app. | SECURITY | | Releases and marketing | Admin-authored notifications, or producers with an audience | RELEASE, MARKETING | diff --git a/src/main/java/com/kingsrook/qbits/alerts/AlertsPreferences.java b/src/main/java/com/kingsrook/qbits/alerts/AlertsPreferences.java index 35f3991..e582c89 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/AlertsPreferences.java +++ b/src/main/java/com/kingsrook/qbits/alerts/AlertsPreferences.java @@ -21,6 +21,7 @@ import java.io.Serializable; import java.util.List; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; +import com.kingsrook.qbits.alerts.engine.AlertRouter; import com.kingsrook.qbits.alerts.engine.AlertsDb; import com.kingsrook.qbits.alerts.model.AlertsPossibleValues.ChannelType; import com.kingsrook.qbits.alerts.model.AlertsPossibleValues.DeliveryMode; @@ -112,7 +113,7 @@ public final class AlertsPreferences .withValue("userId", userId) .withValue("scopeType", ScopeType.ALL.name()) .withValue("minSeverity", runtime.getConfig().getBootstrapMinSeverity().name()) - .withValue("events", "RAISED,ESCALATED,REMINDER,RESOLVED") + .withValue("events", AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS) .withValue("deliveryMode", DeliveryMode.IMMEDIATE.name()) .withValue("channelOverrideIds", String.valueOf(inApp.getValue("id"))) .withValue("enabled", true) diff --git a/src/main/java/com/kingsrook/qbits/alerts/engine/AlertRouter.java b/src/main/java/com/kingsrook/qbits/alerts/engine/AlertRouter.java index 8a267d9..73210a9 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/engine/AlertRouter.java +++ b/src/main/java/com/kingsrook/qbits/alerts/engine/AlertRouter.java @@ -92,15 +92,28 @@ public class AlertRouter private static final int DUPLICATE_WINDOW_MINUTES = 10; + /** The events a new subscription gets unless someone chooses others. */ + public static final String DEFAULT_SUBSCRIPTION_EVENTS = "RAISED,ESCALATED,REMINDER,RESOLVED"; + private final AlertsRuntime runtime; /******************************************************************************* - ** One destination for one notification. + ** One destination for one notification. explicitResolve: the subscription + ** behind it chose resolve notices on purpose (see explicitlyWantsResolved), + ** so a resolve brings the alert back to unread in that person's bell. *******************************************************************************/ - record Destination(String userId, QRecord channel, Integer subscriptionId, String minSeverity, QRecord preference, String deliveryMode) + record Destination(String userId, QRecord channel, Integer subscriptionId, String minSeverity, QRecord preference, String deliveryMode, boolean explicitResolve) { + /*************************************************************************** + ** A destination that is not from a subscription with explicit resolve + ** notices. + ***************************************************************************/ + Destination(String userId, QRecord channel, Integer subscriptionId, String minSeverity, QRecord preference, String deliveryMode) + { + this(userId, channel, subscriptionId, minSeverity, preference, deliveryMode, false); + } } @@ -337,6 +350,14 @@ public class AlertRouter notification.setValue("status", DeliveryStatus.SUPPRESSED.name()); notification.setValue("suppressedReason", reason.name()); AlertsDb.insert(AlertsDelivery.TABLE_NAME, notification); + if(inApp && reason == SuppressedReason.DUPLICATE) + { + /////////////////////////////////////////////////////////////////// + // not a new notice, but keep the bell entry's text in step (a // + // quick reopen must not leave it reading "Resolved") // + /////////////////////////////////////////////////////////////////// + runtime.getNotifications().deliverAlert(destination.userId(), alert, now, false); + } } else if(digest) { @@ -349,12 +370,21 @@ public class AlertRouter } else if(inApp) { + ////////////////////////////////////////////////////////////////////////// + // a resolve is good news, not something to act on: it updates the // + // alert's bell entry in place without marking it unread, unless the // + // person's subscription asked for resolve notices on purpose // + ////////////////////////////////////////////////////////////////////////// + boolean quiet = kind == DeliveryKind.RESOLVED && !destination.explicitResolve(); notification.setValue("status", DeliveryStatus.SENT.name()); notification.setValue("sentAt", now); QRecord inserted = AlertsDb.insert(AlertsDelivery.TABLE_NAME, notification); - runtime.getNotifications().deliverAlert(destination.userId(), alert, now); - LOG.debug("In-app alert delivered", logPair("deliveryId", inserted.getValue("id"))); - queued++; + runtime.getNotifications().deliverAlert(destination.userId(), alert, now, !quiet); + LOG.debug("In-app alert delivered", logPair("deliveryId", inserted.getValue("id")), logPair("quiet", quiet)); + if(!quiet) + { + queued++; + } } else { @@ -471,7 +501,8 @@ public class AlertRouter QRecord preference = AlertsDb.first(AlertsUserPreference.TABLE_NAME, AlertsDb.eq("userId", userId)); for(QRecord channel : userChannels(subscription, preference)) { - destinations.putIfAbsent("user:" + userId + ":" + channel.getValue("id"), new Destination(userId, channel, subscriptionId, minName, preference, subscription.getValueString("deliveryMode"))); + destinations.putIfAbsent("user:" + userId + ":" + channel.getValue("id"), new Destination(userId, channel, subscriptionId, minName, preference, + subscription.getValueString("deliveryMode"), explicitlyWantsResolved(subscription.getValueString("events")))); } } } @@ -781,7 +812,41 @@ public class AlertRouter /******************************************************************************* - ** Whether a subscription's events list includes this kind (empty: all). + ** Whether a subscription asked for resolve notices on purpose: its events + ** name RESOLVED and are not the default set (an empty list means "all + ** kinds", which is not a choice either). + *******************************************************************************/ + static boolean explicitlyWantsResolved(String events) + { + if(!StringUtils.hasContent(events) || !includesKind(events, DeliveryKind.RESOLVED)) + { + return (false); + } + return (!eventSet(events).equals(eventSet(DEFAULT_SUBSCRIPTION_EVENTS))); + } + + + + /******************************************************************************* + ** A subscription's events as an upper-case set. + *******************************************************************************/ + private static Set eventSet(String events) + { + Set set = new java.util.TreeSet<>(); + for(String event : events.split("[,\\s]+")) + { + if(!event.isBlank()) + { + set.add(event.trim().toUpperCase(java.util.Locale.ROOT)); + } + } + return (set); + } + + + + /******************************************************************************* + ** Whether a subscription's events include a kind (empty: all kinds). *******************************************************************************/ private static boolean includesKind(String events, DeliveryKind kind) { diff --git a/src/main/java/com/kingsrook/qbits/alerts/metadata/AlertsTableMetaDataHelper.java b/src/main/java/com/kingsrook/qbits/alerts/metadata/AlertsTableMetaDataHelper.java index 55facb3..6889723 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/metadata/AlertsTableMetaDataHelper.java +++ b/src/main/java/com/kingsrook/qbits/alerts/metadata/AlertsTableMetaDataHelper.java @@ -27,6 +27,7 @@ import com.kingsrook.qbits.alerts.AlertsSecurity; import com.kingsrook.qbits.alerts.customizers.AlertsNotificationCustomizer; import com.kingsrook.qbits.alerts.customizers.AlertsSubscriptionCustomizer; import com.kingsrook.qbits.alerts.customizers.AlertsWriteGuardCustomizer; +import com.kingsrook.qbits.alerts.engine.AlertRouter; import com.kingsrook.qbits.alerts.model.AlertsAlert; import com.kingsrook.qbits.alerts.model.AlertsChannel; import com.kingsrook.qbits.alerts.model.AlertsDelivery; @@ -262,7 +263,7 @@ public final class AlertsTableMetaDataHelper table.getField("scopeType").setDefaultValue(AlertsPossibleValues.ScopeType.ALL.name()); table.getField("minSeverity").setDefaultValue("WARNING"); table.getField("deliveryMode").setDefaultValue(AlertsPossibleValues.DeliveryMode.IMMEDIATE.name()); - table.getField("events").setDefaultValue("RAISED,ESCALATED,REMINDER,RESOLVED"); + table.getField("events").setDefaultValue(AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS); table.getField("enabled").setDefaultValue(true); table.getField("userId").setIsEditable(false); return (customize(table, AlertsSubscriptionCustomizer.class)); diff --git a/src/main/java/com/kingsrook/qbits/alerts/notifications/NotificationService.java b/src/main/java/com/kingsrook/qbits/alerts/notifications/NotificationService.java index b861d1a..2ef7843 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/notifications/NotificationService.java +++ b/src/main/java/com/kingsrook/qbits/alerts/notifications/NotificationService.java @@ -34,6 +34,7 @@ import com.kingsrook.qbits.alerts.model.AlertsAlert; import com.kingsrook.qbits.alerts.model.AlertsNotification; import com.kingsrook.qbits.alerts.model.AlertsNotificationPreference; import com.kingsrook.qbits.alerts.model.AlertsNotificationRecipient; +import com.kingsrook.qbits.alerts.model.AlertsPossibleValues; import com.kingsrook.qqq.backend.core.actions.tables.DeleteAction; import com.kingsrook.qqq.backend.core.actions.tables.InsertAction; import com.kingsrook.qqq.backend.core.actions.tables.UpdateAction; @@ -73,6 +74,9 @@ public class NotificationService public static final String STATUS_SENT = "SENT"; public static final String STATUS_RETRACTED = "RETRACTED"; + /** Put before a resolved alert's title in the bell. */ + public static final String RESOLVED_PREFIX = "Resolved: "; + static final int BATCH = 500; private final AlertsRuntime runtime; @@ -332,11 +336,30 @@ public class NotificationService ** back to unread on every delivery. Call as the system user. *******************************************************************************/ public void deliverAlert(String userId, QRecord alert, Instant now) throws QException + { + deliverAlert(userId, alert, now, true); + } + + + + /******************************************************************************* + ** Put an alert in a user's bell. With markUnread false (a resolve) the + ** alert's existing bell entry is only updated in place, its title marked + ** "Resolved: ", and the user's read state is left as it was; nothing is + ** added when the alert was never in the bell. Returns whether the user's + ** bell entry was written. Call as the system user. + *******************************************************************************/ + public boolean deliverAlert(String userId, QRecord alert, Instant now, boolean markUnread) throws QException { Integer alertId = alert.getValueInteger("id"); String sourceKey = "alert:" + alertId; - QRecord values = new QRecord() - .withValue("title", StringUtils.safeTruncate(alert.getValueString("title"), 200)) + String title = alert.getValueString("title"); + if(!markUnread && AlertsPossibleValues.Status.RESOLVED.name().equals(alert.getValueString("status"))) + { + title = RESOLVED_PREFIX + (title == null ? "" : title); + } + QRecord values = new QRecord() + .withValue("title", StringUtils.safeTruncate(title, 200)) .withValue("body", alert.getValueString("summary")) .withValue("severity", severityOf(alert.getValueString("severity"), NotificationSeverity.WARNING).name()) .withValue("category", NotificationCategory.ALERT.name()) @@ -352,6 +375,10 @@ public class NotificationService Integer notificationId; if(notification == null) { + if(!markUnread) + { + return (false); + } notificationId = AlertsDb.insert(AlertsNotification.TABLE_NAME, values .withValue("audienceType", NotificationAudience.Type.USERS.name()) .withValue("status", STATUS_SENT) @@ -367,6 +394,10 @@ public class NotificationService QRecord recipient = AlertsDb.first(AlertsNotificationRecipient.TABLE_NAME, AlertsDb.eq("notificationId", notificationId) .withCriteria("userId", QCriteriaOperator.EQUALS, userId)); + if(!markUnread) + { + return (recipient != null); + } if(recipient == null) { AlertsDb.insert(AlertsNotificationRecipient.TABLE_NAME, newRecipient(notificationId, userId, now)); @@ -378,6 +409,7 @@ public class NotificationService AlertsDb.update(AlertsNotificationRecipient.TABLE_NAME, new QRecord().withValue("id", recipient.getValue("id")) .withValue("deliveredAt", now).withValue("readAt", null).withValue("dismissedAt", null)); } + return (true); } diff --git a/src/main/java/com/kingsrook/qbits/alerts/processes/AlertsActionSteps.java b/src/main/java/com/kingsrook/qbits/alerts/processes/AlertsActionSteps.java index f556beb..e070ab2 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/processes/AlertsActionSteps.java +++ b/src/main/java/com/kingsrook/qbits/alerts/processes/AlertsActionSteps.java @@ -25,6 +25,7 @@ import java.util.Map; import java.util.Objects; import com.kingsrook.qbits.alerts.AlertsRuntime; import com.kingsrook.qbits.alerts.AlertsSecurity; +import com.kingsrook.qbits.alerts.engine.AlertRouter; import com.kingsrook.qbits.alerts.engine.AlertsDb; import com.kingsrook.qbits.alerts.engine.AlertsEngine; import com.kingsrook.qbits.alerts.engine.MonitorEvaluator; @@ -414,7 +415,7 @@ public final class AlertsActionSteps .withValue("scopeValue", scopeValue) .withValue("minSeverity", StringUtils.hasContent(input.getValueString(AlertsAction.FIELD_MIN_SEVERITY)) ? input.getValueString(AlertsAction.FIELD_MIN_SEVERITY) : "WARNING") .withValue("deliveryMode", StringUtils.hasContent(input.getValueString(AlertsAction.FIELD_DELIVERY_MODE)) ? input.getValueString(AlertsAction.FIELD_DELIVERY_MODE) : "IMMEDIATE") - .withValue("events", "RAISED,ESCALATED,REMINDER,RESOLVED") + .withValue("events", AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS) .withValue("enabled", true))); throwIfErrors(inserted); return (true); diff --git a/src/main/resources/help/qbit-alerts.yaml b/src/main/resources/help/qbit-alerts.yaml index 1bfbd38..0e7d984 100644 --- a/src/main/resources/help/qbit-alerts.yaml +++ b/src/main/resources/help/qbit-alerts.yaml @@ -419,6 +419,7 @@ entries: summary: Which kinds of notification to send, as a comma-separated list; empty means all. content: | Default is {{default:alerts_subscription.events}}. Raised also covers a reopened alert, a severity increase and an alert starting to flap. Reminders and escalations are sent immediately, including for digest subscriptions. + In the bell, a resolve only marks the alert's entry as resolved and leaves it read or unread as it was. To have resolves come back as unread, choose a list that names Resolved and differs from the default, such as Raised and Resolved. - key: table:alerts_subscription;field:scopeValue summary: What the scope matches on; its meaning depends on Scope Type. content: | diff --git a/src/test/java/com/kingsrook/qbits/alerts/engine/QuietNotificationsTest.java b/src/test/java/com/kingsrook/qbits/alerts/engine/QuietNotificationsTest.java new file mode 100644 index 0000000..fb8ecc7 --- /dev/null +++ b/src/test/java/com/kingsrook/qbits/alerts/engine/QuietNotificationsTest.java @@ -0,0 +1,222 @@ +/* + * Copyright 2026 Kingsrook, LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.kingsrook.qbits.alerts.engine; + + +import java.time.Duration; +import java.util.List; +import com.kingsrook.qbits.alerts.BaseAlertsTest; +import com.kingsrook.qbits.alerts.model.AlertsDelivery; +import com.kingsrook.qbits.alerts.model.AlertsNotification; +import com.kingsrook.qbits.alerts.model.AlertsNotificationRecipient; +import com.kingsrook.qbits.alerts.model.AlertsPossibleValues.Resolution; +import com.kingsrook.qbits.alerts.model.AlertsSubscription; +import com.kingsrook.qbits.alerts.notifications.NotificationService; +import com.kingsrook.qqq.backend.core.actions.tables.DeleteAction; +import com.kingsrook.qqq.backend.core.alerts.AlertSeverity; +import com.kingsrook.qqq.backend.core.alerts.AlertSignal; +import com.kingsrook.qqq.backend.core.alerts.AlertsInstanceMetaData; +import com.kingsrook.qqq.backend.core.alerts.QAlertTypeMetaData; +import com.kingsrook.qqq.backend.core.exceptions.QException; +import com.kingsrook.qqq.backend.core.model.actions.tables.delete.DeleteInput; +import com.kingsrook.qqq.backend.core.model.actions.tables.query.QCriteriaOperator; +import com.kingsrook.qqq.backend.core.model.actions.tables.query.QFilterCriteria; +import com.kingsrook.qqq.backend.core.model.actions.tables.query.QQueryFilter; +import com.kingsrook.qqq.backend.core.model.data.QRecord; +import com.kingsrook.qqq.backend.core.model.metadata.QInstance; +import org.junit.jupiter.api.Test; +import static org.assertj.core.api.Assertions.assertThat; + + +/******************************************************************************* + ** Notifications people should see or act on: a resolve updates the bell + ** entry without marking it unread (unless the subscription asked for + ** resolve notices on purpose), and a reopen reuses the same bell entry. + *******************************************************************************/ +class QuietNotificationsTest extends BaseAlertsTest +{ + static final String TYPE = "test.widget.broken"; + + + + /******************************************************************************* + ** + *******************************************************************************/ + @Override + protected void customizeInstance(QInstance qInstance) + { + AlertsInstanceMetaData.ofOrWithNew(qInstance) + .withAlertType(new QAlertTypeMetaData().withName(TYPE).withLabel("Widget broken").withGroupName("widgets").withDefaultSeverity(AlertSeverity.ERROR)); + } + + + + /******************************************************************************* + ** A resolve keeps the bell entry read, marks it resolved, and adds nothing. + *******************************************************************************/ + @Test + void testResolveDoesNotMarkTheBellUnread() throws Exception + { + subscribe(ADMIN, AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS); + QRecord alert = runtime.getEngine().raise(new AlertSignal(TYPE).withDedupKey("q1").withTitle("Widget q1 is broken")); + Integer id = alert.getValueInteger("id"); + + QRecord recipient = recipient(ADMIN); + assertThat(recipient.getValue("readAt")).isNull(); + markRead(recipient); + + clock.advance(Duration.ofMinutes(5)); + runtime.getEngine().resolve(TYPE, "q1", Resolution.PRODUCER, null, "fixed"); + + assertThat(all(AlertsNotification.TABLE_NAME)).singleElement() + .matches(n -> (NotificationService.RESOLVED_PREFIX + "Widget q1 is broken").equals(n.getValueString("title"))); + assertThat(all(AlertsNotificationRecipient.TABLE_NAME)).hasSize(1); + assertThat(recipient(ADMIN).getValue("readAt")).isNotNull(); + + //////////////////////////////////////////////////////////////////// + // a reopen is news again: same bell entry, unread, title restored // + // (past the router's 10-minute duplicate window) // + //////////////////////////////////////////////////////////////////// + clock.advance(Duration.ofMinutes(5)); + ageDeliveries(); + QRecord reopened = runtime.getEngine().raise(new AlertSignal(TYPE).withDedupKey("q1").withTitle("Widget q1 is broken")); + assertThat(reopened.getValueInteger("id")).isEqualTo(id); + assertThat(all(AlertsNotification.TABLE_NAME)).singleElement().matches(n -> "Widget q1 is broken".equals(n.getValueString("title"))); + assertThat(all(AlertsNotificationRecipient.TABLE_NAME)).hasSize(1); + assertThat(recipient(ADMIN).getValue("readAt")).isNull(); + } + + + + /******************************************************************************* + ** An unread entry stays unread through a resolve, and a resolve never adds + ** an entry for someone whose bell did not have the alert. + *******************************************************************************/ + @Test + void testResolveLeavesUnreadAndAddsNothing() throws Exception + { + subscribe(ADMIN, ""); + runtime.getEngine().raise(new AlertSignal(TYPE).withDedupKey("q2").withTitle("Widget q2 is broken")); + runtime.asSystem(() -> AlertsDb.insert(AlertsSubscription.TABLE_NAME, new QRecord().withValue("subscriberType", "USER").withValue("userId", MANAGER) + .withValue("scopeType", "ALL").withValue("minSeverity", "WARNING").withValue("events", AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS) + .withValue("deliveryMode", "IMMEDIATE").withValue("enabled", true))); + + runtime.getEngine().resolve(TYPE, "q2", Resolution.PRODUCER, null, null); + assertThat(recipient(ADMIN).getValue("readAt")).isNull(); + assertThat(all(AlertsNotificationRecipient.TABLE_NAME)).extracting(r -> r.getValueString("userId")).containsExactly(ADMIN); + } + + + + /******************************************************************************* + ** A subscription that chose resolve notices on purpose keeps the old + ** behavior: the resolve brings the entry back to unread. + *******************************************************************************/ + @Test + void testExplicitResolveSubscriptionMarksUnread() throws Exception + { + subscribe(ADMIN, "RAISED,RESOLVED"); + runtime.getEngine().raise(new AlertSignal(TYPE).withDedupKey("q3").withTitle("Widget q3 is broken")); + markRead(recipient(ADMIN)); + + runtime.getEngine().resolve(TYPE, "q3", Resolution.PRODUCER, null, null); + assertThat(recipient(ADMIN).getValue("readAt")).isNull(); + assertThat(all(AlertsNotification.TABLE_NAME)).singleElement().matches(n -> "Widget q3 is broken".equals(n.getValueString("title"))); + } + + + + /******************************************************************************* + ** A reopen inside the duplicate window is not a new notice, but the bell + ** entry no longer reads "Resolved". + *******************************************************************************/ + @Test + void testQuickReopenRestoresTheTitle() throws Exception + { + subscribe(ADMIN, AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS); + runtime.getEngine().raise(new AlertSignal(TYPE).withDedupKey("q4").withTitle("Widget q4 is broken")); + markRead(recipient(ADMIN)); + runtime.getEngine().resolve(TYPE, "q4", Resolution.PRODUCER, null, null); + runtime.getEngine().raise(new AlertSignal(TYPE).withDedupKey("q4").withTitle("Widget q4 is broken")); + + assertThat(all(AlertsNotification.TABLE_NAME)).singleElement().matches(n -> "Widget q4 is broken".equals(n.getValueString("title"))); + assertThat(recipient(ADMIN).getValue("readAt")).isNotNull(); + } + + + + /******************************************************************************* + ** Which event lists count as an explicit choice of resolve notices. + *******************************************************************************/ + @Test + void testExplicitlyWantsResolved() + { + assertThat(AlertRouter.explicitlyWantsResolved(null)).isFalse(); + assertThat(AlertRouter.explicitlyWantsResolved("")).isFalse(); + assertThat(AlertRouter.explicitlyWantsResolved(AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS)).isFalse(); + assertThat(AlertRouter.explicitlyWantsResolved("resolved, reminder ,ESCALATED,raised")).isFalse(); + assertThat(AlertRouter.explicitlyWantsResolved("RAISED,ESCALATED")).isFalse(); + assertThat(AlertRouter.explicitlyWantsResolved("RAISED,RESOLVED")).isTrue(); + assertThat(AlertRouter.explicitlyWantsResolved("RESOLVED")).isTrue(); + } + + + + /******************************************************************************* + ** + *******************************************************************************/ + private void subscribe(String userId, String events) throws QException + { + runtime.asSystem(() -> AlertsDb.insert(AlertsSubscription.TABLE_NAME, new QRecord().withValue("subscriberType", "USER").withValue("userId", userId) + .withValue("scopeType", "ALL").withValue("minSeverity", "WARNING").withValue("events", events) + .withValue("deliveryMode", "IMMEDIATE").withValue("enabled", true))); + } + + + + /******************************************************************************* + ** + *******************************************************************************/ + private QRecord recipient(String userId) throws QException + { + List rows = where(AlertsNotificationRecipient.TABLE_NAME, AlertsDb.eq("userId", userId)); + assertThat(rows).hasSize(1); + return (rows.get(0)); + } + + + + /******************************************************************************* + ** Move past the router's duplicate window. Its check reads createDate, + ** which is wall-clock time, so the earlier deliveries are cleared instead. + *******************************************************************************/ + private void ageDeliveries() throws QException + { + runtime.asSystem(() -> new DeleteAction().execute(new DeleteInput(AlertsDelivery.TABLE_NAME) + .withQueryFilter(new QQueryFilter(new QFilterCriteria("id", QCriteriaOperator.IS_NOT_BLANK))))); + } + + + + /******************************************************************************* + ** + *******************************************************************************/ + private void markRead(QRecord recipient) throws QException + { + runtime.asSystem(() -> AlertsDb.update(AlertsNotificationRecipient.TABLE_NAME, new QRecord().withValue("id", recipient.getValue("id")).withValue("readAt", clock.instant()))); + } +} -- 2.54.0 From c106385710292aba21f19dcb39c2a0aa099b3359 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 16 Oct 2026 15:44:24 -0500 Subject: [PATCH 2/5] feat(engine): per-type reopen window, 6 hours by default for health checks A health blip more than 30 minutes after the last resolve created a new alert and a second bell entry for the same problem. The reopen window is now resolved per type: alerts_type.reopenWindowMinutes (changeset 55), else the longest matching prefix in AlertsQBitConfig.reopenWindowMinutesByTypePrefix (default health. = 360 minutes), else reopenWindowMinutes (30). A reopen updates the existing alert and its bell entry; flapping detection is unchanged. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + UPGRADE.md | 7 ++ .../qbits/alerts/AlertsQBitConfig.java | 83 +++++++++++++++- .../qbits/alerts/engine/AlertsEngine.java | 29 +++++- .../metadata/AlertsTableMetaDataHelper.java | 2 +- .../qbits/alerts/model/AlertsType.java | 34 +++++++ .../qbit-alerts/alerts.changelog.sql | 6 ++ src/main/resources/help/qbit-alerts.yaml | 4 + .../alerts/engine/QuietNotificationsTest.java | 97 ++++++++++++++++++- src/test/resources/alerts-h2.sql | 1 + 10 files changed, 254 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b387250..f56b248 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to this project are documented here. The format follows Keep ## [0.1.0] - Unreleased ### Fixed +- Duplicate health alerts: a health check that failed again more than 30 minutes after its last resolve created a second alert and a second bell entry for the same problem. The reopen window is now per type: `alerts_type.reopenWindowMinutes` (changeset 55), else the longest matching prefix in `AlertsQBitConfig.reopenWindowMinutesByTypePrefix` (default `health.` = 360 minutes), else `reopenWindowMinutes` (30). A reopen updates the existing alert and its bell entry; flapping detection is unchanged. - Bell noise: a resolve marked the alert's bell entry unread again. It now updates the entry in place (title "Resolved: ...") and leaves the read state alone. A subscription whose events name RESOLVED and differ from the default set (`AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS`) keeps the old behavior. External channels are unchanged. - Under OAuth2, per-recipient checks (alert visibility for external channels, follower record access, audience sessions) never worked: they used token-less automated sessions, which OAuth2 rejects, so external sends fell back to the conservative rule. When the host declares the QQQ `alerts` service principal (`AlertsQBitConfig.withServicePrincipal`, default `alerts`), these checks run on the recipient's behalf through it: the recipient's current access intersected with the principal's. Alert recording and routing bookkeeping stay on the system session, so alerting does not depend on the identity provider being up. With no principal declared, behavior is unchanged. Needs the QQQ release with service identities (qqq#1255, #1256). - Audit noise: every monitor run (lease claim, then run result) and every reminder check wrote a "Record was Edited" audit (about 7,000 `alerts_monitor` and 1,800 `alerts_alert` audits in 6 hours on one production host, most with no field details). Engine updates that touch only bookkeeping fields (`RuntimeFields`: monitor `claimedBy`, `claimedUntil`, `lastRunAt`, `nextRunAt`, `lastStatus`, `lastValue`, `lastError`, `lastElapsedMs`, `consecutiveFailures`, `conditionSince`; alert `timedNotificationCheckedAt`, `lastNotifiedAt`, `recordRefCount`) are written with `omitDmlAudit`. Configuration edits, raises and alert state changes are audited as before, as are UI and API edits of any field. diff --git a/UPGRADE.md b/UPGRADE.md index 3189b3a..0990dea 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,12 @@ # Upgrading +## Unreleased: quieter notifications + +1. **Run the changelog as usual.** Changeset 55 adds the nullable `alerts_type.reopen_window_minutes` column (guarded, additive). +2. **Behavior changes to know about.** + - A resolve no longer marks the alert's bell entry unread; the entry is updated in place ("Resolved: ..."). A subscription whose events name RESOLVED and differ from the default set keeps the old behavior. Email, Slack and webhook resolve notices are unchanged. + - The reopen window is now per alert type: an alert type's **Reopen Window Minutes**, else `AlertsQBitConfig.withReopenWindowMinutesByTypePrefix` (default `health.` = 360 minutes), else `withReopenWindowMinutes` (default 30). A health check that blips again within 6 hours reopens the same alert and bell entry instead of creating a new one. + ## Unreleased: starter defaults, setup card, table help 1. **Add the defaults changelog** after `alerts.changelog.sql` in the host's master changelog: diff --git a/src/main/java/com/kingsrook/qbits/alerts/AlertsQBitConfig.java b/src/main/java/com/kingsrook/qbits/alerts/AlertsQBitConfig.java index 4e330f5..ee22672 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/AlertsQBitConfig.java +++ b/src/main/java/com/kingsrook/qbits/alerts/AlertsQBitConfig.java @@ -18,7 +18,9 @@ package com.kingsrook.qbits.alerts; import java.util.ArrayList; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import com.fasterxml.jackson.annotation.JsonIgnore; import com.kingsrook.qbits.alerts.model.AlertsAlert; import com.kingsrook.qbits.alerts.model.AlertsChannel; @@ -77,6 +79,9 @@ public class AlertsQBitConfig implements QBitConfig public static final String APP_NAME = "alerts"; + /** Alert types named with this prefix are health checks (6-hour reopen window by default). */ + public static final String HEALTH_TYPE_PREFIX = "health."; + private Boolean legacyAlertmanagerPath = false; private String backendName; @@ -88,6 +93,8 @@ public class AlertsQBitConfig implements QBitConfig private String publicBaseUrl; private Integer reopenWindowMinutes = 30; + + private Map reopenWindowMinutesByTypePrefix = new LinkedHashMap<>(Map.of(HEALTH_TYPE_PREFIX, 360)); private Integer flapThreshold = 5; private Integer flapWindowMinutes = 60; private Integer maxOpenPerMonitor = 50; @@ -163,6 +170,14 @@ public class AlertsQBitConfig implements QBitConfig { assertCondition(value != null && value > 0, "Alerts QBit timing and limit settings must be positive", errors); } + if(reopenWindowMinutesByTypePrefix != null) + { + for(Map.Entry entry : reopenWindowMinutesByTypePrefix.entrySet()) + { + assertCondition(StringUtils.hasContent(entry.getKey()) && entry.getValue() != null && entry.getValue() > 0, + "Alerts QBit reopen windows by type prefix need a prefix and a positive number of minutes", errors); + } + } } @@ -424,7 +439,8 @@ public class AlertsQBitConfig implements QBitConfig /******************************************************************************* ** Fluent setter for reopenWindowMinutes (a raise this soon after a resolve - ** reopens the same alert; default 30) + ** reopens the same alert; default 30, for types that no prefix in + ** reopenWindowMinutesByTypePrefix matches and that set no window of their own) *******************************************************************************/ public AlertsQBitConfig withReopenWindowMinutes(Integer reopenWindowMinutes) { @@ -434,6 +450,71 @@ public class AlertsQBitConfig implements QBitConfig + /******************************************************************************* + ** Getter for reopenWindowMinutesByTypePrefix + *******************************************************************************/ + public Map getReopenWindowMinutesByTypePrefix() + { + return (this.reopenWindowMinutesByTypePrefix); + } + + + + /******************************************************************************* + ** Fluent setter for reopenWindowMinutesByTypePrefix: default reopen windows + ** for alert types whose names start with a prefix (the longest matching + ** prefix wins). Default: health.* types reopen within 360 minutes, so a + ** brief health blip hours later reopens the same alert. A type's own + ** reopenWindowMinutes (alerts_type) wins over this. + *******************************************************************************/ + public AlertsQBitConfig withReopenWindowMinutesByTypePrefix(Map reopenWindowMinutesByTypePrefix) + { + this.reopenWindowMinutesByTypePrefix = reopenWindowMinutesByTypePrefix; + return (this); + } + + + + /******************************************************************************* + ** Set the default reopen window for alert types whose names start with a + ** prefix. + *******************************************************************************/ + public AlertsQBitConfig withReopenWindowMinutesForTypePrefix(String prefix, Integer minutes) + { + if(this.reopenWindowMinutesByTypePrefix == null) + { + this.reopenWindowMinutesByTypePrefix = new LinkedHashMap<>(); + } + this.reopenWindowMinutesByTypePrefix.put(prefix, minutes); + return (this); + } + + + + /******************************************************************************* + ** The default reopen window for an alert type: the longest matching + ** prefix's, else reopenWindowMinutes. + *******************************************************************************/ + public int reopenWindowMinutesFor(String alertTypeName) + { + String longest = null; + Integer minutes = reopenWindowMinutes; + if(alertTypeName != null && reopenWindowMinutesByTypePrefix != null) + { + for(Map.Entry entry : reopenWindowMinutesByTypePrefix.entrySet()) + { + if(entry.getKey() != null && entry.getValue() != null && alertTypeName.startsWith(entry.getKey()) && (longest == null || entry.getKey().length() > longest.length())) + { + longest = entry.getKey(); + minutes = entry.getValue(); + } + } + } + return (minutes == null ? 30 : minutes); + } + + + /******************************************************************************* ** Getter for flapThreshold *******************************************************************************/ diff --git a/src/main/java/com/kingsrook/qbits/alerts/engine/AlertsEngine.java b/src/main/java/com/kingsrook/qbits/alerts/engine/AlertsEngine.java index dcc97d4..55119de 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/engine/AlertsEngine.java +++ b/src/main/java/com/kingsrook/qbits/alerts/engine/AlertsEngine.java @@ -63,8 +63,10 @@ import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair; ** - Raise on an open (type, dedupKey) updates that alert: lastRaisedAt, ** occurrences, record refs. A severity increase records SEVERITY_CHANGED, ** unsnoozes and notifies again; a decrease only records it. - ** - Raise on a key resolved less than reopenWindowMinutes ago reopens the - ** same alert; later, a new alert is created (history stays on the old one). + ** - Raise on a key resolved within the type's reopen window (its own + ** reopenWindowMinutes, else 6 hours for health.* types and 30 minutes for + ** others, both configurable) reopens the same alert and its bell entry; + ** later, a new alert is created (history stays on the old one). ** - Flapping: flapThreshold open/resolve changes within flapWindowMinutes ** set flapping (one notice; further open/resolve notifications are ** suppressed until it is stable for the window). @@ -155,7 +157,7 @@ public class AlertsEngine } else { - QRecord recent = recentlyResolved(signal.getAlertTypeName(), dedupKey, now); + QRecord recent = recentlyResolved(type, signal.getAlertTypeName(), dedupKey, now); if(recent != null) { alert = reopen(recent, type, signal, severity, monitorId, now, null, "Raised again within the reopen window"); @@ -680,9 +682,9 @@ public class AlertsEngine /******************************************************************************* ** The most recent alert for (type, key) resolved within the reopen window. *******************************************************************************/ - private QRecord recentlyResolved(String alertTypeName, String dedupKey, Instant now) throws QException + private QRecord recentlyResolved(QRecord type, String alertTypeName, String dedupKey, Instant now) throws QException { - Instant cutoff = now.minus(runtime.getConfig().getReopenWindowMinutes(), ChronoUnit.MINUTES); + Instant cutoff = now.minus(reopenWindowMinutes(type, alertTypeName), ChronoUnit.MINUTES); return (AlertsDb.first(AlertsAlert.TABLE_NAME, AlertsDb.eq("alertTypeName", alertTypeName) .withCriteria("dedupKey", QCriteriaOperator.EQUALS, dedupKey) .withCriteria("status", QCriteriaOperator.EQUALS, Status.RESOLVED.name()) @@ -692,6 +694,23 @@ public class AlertsEngine + /******************************************************************************* + ** How long after a resolve a raise of the same (type, key) reopens the + ** alert: the type's own reopenWindowMinutes, else the configured default + ** for its name (6 hours for health.* types, 30 minutes otherwise). + *******************************************************************************/ + int reopenWindowMinutes(QRecord type, String alertTypeName) + { + Integer perType = type == null ? null : type.getValueInteger("reopenWindowMinutes"); + if(perType != null && perType > 0) + { + return (perType); + } + return (runtime.getConfig().reopenWindowMinutesFor(alertTypeName)); + } + + + /******************************************************************************* ** Record a REPEATED entry at most once an hour per alert. *******************************************************************************/ diff --git a/src/main/java/com/kingsrook/qbits/alerts/metadata/AlertsTableMetaDataHelper.java b/src/main/java/com/kingsrook/qbits/alerts/metadata/AlertsTableMetaDataHelper.java index 6889723..5128f37 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/metadata/AlertsTableMetaDataHelper.java +++ b/src/main/java/com/kingsrook/qbits/alerts/metadata/AlertsTableMetaDataHelper.java @@ -159,7 +159,7 @@ public final class AlertsTableMetaDataHelper .withSection(new QFieldSection("identity", new QIcon("badge"), Tier.T1, List.of("id", "label", "name", "groupId", "sourceName", "openCount"))) .withSection(new QFieldSection("about", "About", new QIcon("info"), Tier.T2, List.of("description", "howToFix", "subjectTableName", "visibilityPermission"))) .withSection(new QFieldSection("tuning", "Tuning", new QIcon("tune"), Tier.T2, List.of("enabled", "notify", "defaultSeverity", "severityOverride", "resolveMode", - "quietPeriodMinutes", "reminderIntervalMinutes", "escalationPolicyId", "externalDetailCeiling"))) + "quietPeriodMinutes", "reopenWindowMinutes", "reminderIntervalMinutes", "escalationPolicyId", "externalDetailCeiling"))) .withSection(widgetSection(JOIN_TYPE_MONITOR, "Monitors", "monitor_heart")) .withSection(widgetSection(JOIN_TYPE_ALERT, "Alerts", "notifications_active")) .withSection(new QFieldSection("system", new QIcon("calendar_month"), Tier.T3, List.of("installed", "lastRaisedAt", "createDate", "modifyDate"))); diff --git a/src/main/java/com/kingsrook/qbits/alerts/model/AlertsType.java b/src/main/java/com/kingsrook/qbits/alerts/model/AlertsType.java index 628adac..5423eb0 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/model/AlertsType.java +++ b/src/main/java/com/kingsrook/qbits/alerts/model/AlertsType.java @@ -90,6 +90,9 @@ public class AlertsType extends QRecordEntity @QField() private Integer reminderIntervalMinutes; + @QField() + private Integer reopenWindowMinutes; + @QField(isEditable = false) private Instant lastRaisedAt; @@ -684,6 +687,37 @@ public class AlertsType extends QRecordEntity + /******************************************************************************* + ** Getter for reopenWindowMinutes + *******************************************************************************/ + public Integer getReopenWindowMinutes() + { + return (this.reopenWindowMinutes); + } + + + + /******************************************************************************* + ** Setter for reopenWindowMinutes + *******************************************************************************/ + public void setReopenWindowMinutes(Integer reopenWindowMinutes) + { + this.reopenWindowMinutes = reopenWindowMinutes; + } + + + + /******************************************************************************* + ** Fluent setter for reopenWindowMinutes + *******************************************************************************/ + public AlertsType withReopenWindowMinutes(Integer reopenWindowMinutes) + { + this.reopenWindowMinutes = reopenWindowMinutes; + return (this); + } + + + /******************************************************************************* ** Getter for lastRaisedAt *******************************************************************************/ diff --git a/src/main/resources/db/liquibase/qbit-alerts/alerts.changelog.sql b/src/main/resources/db/liquibase/qbit-alerts/alerts.changelog.sql index 6a0406b..9df1349 100644 --- a/src/main/resources/db/liquibase/qbit-alerts/alerts.changelog.sql +++ b/src/main/resources/db/liquibase/qbit-alerts/alerts.changelog.sql @@ -775,3 +775,9 @@ INSERT INTO qqq_fk_quarantine (table_name, column_name, row_id, missing_parent_i DELETE FROM alerts_escalation_step c WHERE c.channel_id IS NOT NULL AND NOT EXISTS (SELECT 1 FROM alerts_channel p WHERE p.id = c.channel_id); ALTER TABLE alerts_escalation_step ADD CONSTRAINT fk_alerts_escalation_step_channel_id FOREIGN KEY (channel_id) REFERENCES alerts_channel (id) ON DELETE RESTRICT; -- rollback ALTER TABLE alerts_escalation_step DROP CONSTRAINT IF EXISTS fk_alerts_escalation_step_channel_id; + +-- changeset qbit-alerts:55 +-- comment: Per-type reopen window: a raise this many minutes after a resolve reopens the same alert. Empty uses the qbit's default for the type. +-- preconditions onFail:MARK_RAN onError:HALT +-- precondition-sql-check expectedResult:0 SELECT COUNT(*) FROM information_schema.columns WHERE LOWER(table_name) = 'alerts_type' AND LOWER(column_name) = 'reopen_window_minutes' +ALTER TABLE alerts_type ADD COLUMN reopen_window_minutes INTEGER; diff --git a/src/main/resources/help/qbit-alerts.yaml b/src/main/resources/help/qbit-alerts.yaml index 0e7d984..8e7217e 100644 --- a/src/main/resources/help/qbit-alerts.yaml +++ b/src/main/resources/help/qbit-alerts.yaml @@ -321,6 +321,10 @@ entries: summary: Minutes between reminders while an alert remains open and unacknowledged. content: | Set zero to disable reminders. If blank, Critical alerts default to 60 minutes; other severities have no reminders. Acknowledgement, snooze, resolution and suppression stop timed notifications. +- key: table:alerts_type;field:reopenWindowMinutes + summary: Minutes after a resolve during which the same problem reopens its alert instead of starting a new one. + content: | + Empty uses the default: 6 hours for health checks (types named health.*) and 30 minutes for the rest, set by the application. A reopen updates the existing alert and its bell entry, so a short blip does not show up twice. - key: table:alerts_type;field:quietPeriodMinutes summary: Minutes with no new reports before an alert resolves itself; used when Resolve Mode is Quiet Period. content: | diff --git a/src/test/java/com/kingsrook/qbits/alerts/engine/QuietNotificationsTest.java b/src/test/java/com/kingsrook/qbits/alerts/engine/QuietNotificationsTest.java index fb8ecc7..c084a07 100644 --- a/src/test/java/com/kingsrook/qbits/alerts/engine/QuietNotificationsTest.java +++ b/src/test/java/com/kingsrook/qbits/alerts/engine/QuietNotificationsTest.java @@ -19,12 +19,15 @@ package com.kingsrook.qbits.alerts.engine; import java.time.Duration; import java.util.List; +import com.kingsrook.qbits.alerts.AlertsQBitConfig; import com.kingsrook.qbits.alerts.BaseAlertsTest; +import com.kingsrook.qbits.alerts.model.AlertsAlert; import com.kingsrook.qbits.alerts.model.AlertsDelivery; import com.kingsrook.qbits.alerts.model.AlertsNotification; import com.kingsrook.qbits.alerts.model.AlertsNotificationRecipient; import com.kingsrook.qbits.alerts.model.AlertsPossibleValues.Resolution; import com.kingsrook.qbits.alerts.model.AlertsSubscription; +import com.kingsrook.qbits.alerts.model.AlertsType; import com.kingsrook.qbits.alerts.notifications.NotificationService; import com.kingsrook.qqq.backend.core.actions.tables.DeleteAction; import com.kingsrook.qqq.backend.core.alerts.AlertSeverity; @@ -45,11 +48,13 @@ import static org.assertj.core.api.Assertions.assertThat; /******************************************************************************* ** Notifications people should see or act on: a resolve updates the bell ** entry without marking it unread (unless the subscription asked for - ** resolve notices on purpose), and a reopen reuses the same bell entry. + ** resolve notices on purpose), a reopen reuses the same bell entry, and + ** health types reopen within six hours instead of 30 minutes. *******************************************************************************/ class QuietNotificationsTest extends BaseAlertsTest { - static final String TYPE = "test.widget.broken"; + static final String TYPE = "test.widget.broken"; + static final String HEALTH = "health.database.degraded"; @@ -60,7 +65,8 @@ class QuietNotificationsTest extends BaseAlertsTest protected void customizeInstance(QInstance qInstance) { AlertsInstanceMetaData.ofOrWithNew(qInstance) - .withAlertType(new QAlertTypeMetaData().withName(TYPE).withLabel("Widget broken").withGroupName("widgets").withDefaultSeverity(AlertSeverity.ERROR)); + .withAlertType(new QAlertTypeMetaData().withName(TYPE).withLabel("Widget broken").withGroupName("widgets").withDefaultSeverity(AlertSeverity.ERROR)) + .withAlertType(new QAlertTypeMetaData().withName(HEALTH).withLabel("Database degraded").withGroupName("health").withDefaultSeverity(AlertSeverity.ERROR)); } @@ -159,6 +165,91 @@ class QuietNotificationsTest extends BaseAlertsTest + /******************************************************************************* + ** A health blip hours after the last one reopens the same alert and bell + ** entry instead of adding a second "Database is degraded". + *******************************************************************************/ + @Test + void testHealthBlipReopensWithinSixHours() throws Exception + { + subscribe(ADMIN, AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS); + QRecord first = runtime.getEngine().raise(new AlertSignal(HEALTH).withDedupKey("db1").withTitle("Database db1 is degraded")); + runtime.getEngine().resolve(HEALTH, "db1", Resolution.PRODUCER, null, null); + markRead(recipient(ADMIN)); + + clock.advance(Duration.ofHours(2)); + ageDeliveries(); + QRecord again = runtime.getEngine().raise(new AlertSignal(HEALTH).withDedupKey("db1").withTitle("Database db1 is degraded")); + assertThat(again.getValue("id")).isEqualTo(first.getValue("id")); + assertThat(again.getValueInteger("reopenCount")).isEqualTo(1); + assertThat(all(AlertsAlert.TABLE_NAME)).hasSize(1); + assertThat(all(AlertsNotification.TABLE_NAME)).singleElement().matches(n -> "Database db1 is degraded".equals(n.getValueString("title"))); + assertThat(recipient(ADMIN).getValue("readAt")).isNull(); + + //////////////////////////////////////////////////////// + // past six hours it is a new problem: a new alert // + //////////////////////////////////////////////////////// + runtime.getEngine().resolve(HEALTH, "db1", Resolution.PRODUCER, null, null); + clock.advance(Duration.ofHours(7)); + QRecord fresh = runtime.getEngine().raise(new AlertSignal(HEALTH).withDedupKey("db1").withTitle("Database db1 is degraded")); + assertThat(fresh.getValue("id")).isNotEqualTo(first.getValue("id")); + + ////////////////////////////////////////////////////////////// + // other types keep the 30-minute window // + ////////////////////////////////////////////////////////////// + QRecord widget = runtime.getEngine().raise(new AlertSignal(TYPE).withDedupKey("w9")); + runtime.getEngine().resolve(TYPE, "w9", Resolution.PRODUCER, null, null); + clock.advance(Duration.ofHours(2)); + assertThat(runtime.getEngine().raise(new AlertSignal(TYPE).withDedupKey("w9")).getValue("id")).isNotEqualTo(widget.getValue("id")); + } + + + + /******************************************************************************* + ** A type's own reopen window wins over the default for its name. + *******************************************************************************/ + @Test + void testPerTypeReopenWindow() throws Exception + { + QRecord widget = runtime.getEngine().raise(new AlertSignal(TYPE).withDedupKey("w8")); + runtime.asSystem(() -> AlertsDb.update(AlertsType.TABLE_NAME, new QRecord().withValue("id", widget.getValue("alertTypeId")).withValue("reopenWindowMinutes", 240))); + runtime.getEngine().resolve(TYPE, "w8", Resolution.PRODUCER, null, null); + clock.advance(Duration.ofHours(3)); + assertThat(runtime.getEngine().raise(new AlertSignal(TYPE).withDedupKey("w8")).getValue("id")).isEqualTo(widget.getValue("id")); + + QRecord health = runtime.getEngine().raise(new AlertSignal(HEALTH).withDedupKey("db2")); + runtime.asSystem(() -> AlertsDb.update(AlertsType.TABLE_NAME, new QRecord().withValue("id", health.getValue("alertTypeId")).withValue("reopenWindowMinutes", 10))); + runtime.getEngine().resolve(HEALTH, "db2", Resolution.PRODUCER, null, null); + clock.advance(Duration.ofMinutes(20)); + assertThat(runtime.getEngine().raise(new AlertSignal(HEALTH).withDedupKey("db2")).getValue("id")).isNotEqualTo(health.getValue("id")); + } + + + + /******************************************************************************* + ** Default windows by name prefix: the longest match wins. + *******************************************************************************/ + @Test + void testReopenWindowByTypePrefix() + { + AlertsQBitConfig defaults = new AlertsQBitConfig(); + assertThat(defaults.reopenWindowMinutesFor("health.database.degraded")).isEqualTo(360); + assertThat(defaults.reopenWindowMinutesFor("test.widget.broken")).isEqualTo(30); + assertThat(defaults.reopenWindowMinutesFor(null)).isEqualTo(30); + + AlertsQBitConfig custom = new AlertsQBitConfig().withReopenWindowMinutes(15).withReopenWindowMinutesForTypePrefix("health.database.", 60); + assertThat(custom.reopenWindowMinutesFor("health.database.degraded")).isEqualTo(60); + assertThat(custom.reopenWindowMinutesFor("health.queue.slow")).isEqualTo(360); + assertThat(custom.reopenWindowMinutesFor("other")).isEqualTo(15); + assertThat(new AlertsQBitConfig().withReopenWindowMinutesByTypePrefix(null).reopenWindowMinutesFor("health.x")).isEqualTo(30); + + List errors = new java.util.ArrayList<>(); + new AlertsQBitConfig().withBackendName(BACKEND_NAME).withReopenWindowMinutesForTypePrefix("health.", 0).validate(qInstance, errors); + assertThat(errors).anyMatch(e -> e.contains("reopen windows by type prefix")); + } + + + /******************************************************************************* ** Which event lists count as an explicit choice of resolve notices. *******************************************************************************/ diff --git a/src/test/resources/alerts-h2.sql b/src/test/resources/alerts-h2.sql index f12a708..cf85e37 100644 --- a/src/test/resources/alerts-h2.sql +++ b/src/test/resources/alerts-h2.sql @@ -55,6 +55,7 @@ CREATE TABLE IF NOT EXISTS alerts_type ( quiet_period_minutes INTEGER, external_detail_ceiling VARCHAR(16), reminder_interval_minutes INTEGER, + reopen_window_minutes INTEGER, last_raised_at TIMESTAMP, open_count INTEGER, create_date TIMESTAMP, -- 2.54.0 From f9d51e67e1d97f98d32da1167ad569ed49627586 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 16 Oct 2026 15:47:23 -0500 Subject: [PATCH 3/5] fix(sink): resolve the overflow alert once the queue has drained "Alert signals are being dropped" had no dedup key, was manual-resolve and was never re-armed, so it stayed open forever. It now uses a stable dedup key (the key earlier versions used implicitly, so an alert they left open resolves too). The drain thread resolves it once the queue has stayed below half full for a minute and re-arms it, so a later overflow raises it again. Still ERROR and admin-only. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + UPGRADE.md | 1 + .../qbits/alerts/AlertsQBitProducer.java | 6 +- .../qbits/alerts/engine/QbitAlertsSink.java | 99 +++++++++++++++++-- .../qbits/alerts/engine/SinkOverflowTest.java | 92 +++++++++++++++++ 5 files changed, 187 insertions(+), 12 deletions(-) create mode 100644 src/test/java/com/kingsrook/qbits/alerts/engine/SinkOverflowTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index f56b248..a09f557 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to this project are documented here. The format follows Keep ## [0.1.0] - Unreleased ### Fixed +- "Alert signals are being dropped" (`alerts.sink.overflow`) stayed open forever: it had no dedup key, was manual-resolve, and the sink never re-armed it. It now has a stable dedup key, and the drain thread resolves it once the queue has stayed below half full for a minute, so a later overflow raises it again. Still ERROR, admins only. - Duplicate health alerts: a health check that failed again more than 30 minutes after its last resolve created a second alert and a second bell entry for the same problem. The reopen window is now per type: `alerts_type.reopenWindowMinutes` (changeset 55), else the longest matching prefix in `AlertsQBitConfig.reopenWindowMinutesByTypePrefix` (default `health.` = 360 minutes), else `reopenWindowMinutes` (30). A reopen updates the existing alert and its bell entry; flapping detection is unchanged. - Bell noise: a resolve marked the alert's bell entry unread again. It now updates the entry in place (title "Resolved: ...") and leaves the read state alone. A subscription whose events name RESOLVED and differ from the default set (`AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS`) keeps the old behavior. External channels are unchanged. - Under OAuth2, per-recipient checks (alert visibility for external channels, follower record access, audience sessions) never worked: they used token-less automated sessions, which OAuth2 rejects, so external sends fell back to the conservative rule. When the host declares the QQQ `alerts` service principal (`AlertsQBitConfig.withServicePrincipal`, default `alerts`), these checks run on the recipient's behalf through it: the recipient's current access intersected with the principal's. Alert recording and routing bookkeeping stay on the system session, so alerting does not depend on the identity provider being up. With no principal declared, behavior is unchanged. Needs the QQQ release with service identities (qqq#1255, #1256). diff --git a/UPGRADE.md b/UPGRADE.md index 0990dea..29bba26 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -5,6 +5,7 @@ 1. **Run the changelog as usual.** Changeset 55 adds the nullable `alerts_type.reopen_window_minutes` column (guarded, additive). 2. **Behavior changes to know about.** - A resolve no longer marks the alert's bell entry unread; the entry is updated in place ("Resolved: ..."). A subscription whose events name RESOLVED and differ from the default set keeps the old behavior. Email, Slack and webhook resolve notices are unchanged. + - "Alert signals dropped" (`alerts.sink.overflow`) now resolves itself once the queue has stayed below half full for a minute. An installed type row keeps its stored Resolve Mode label; the behavior does not depend on it. - The reopen window is now per alert type: an alert type's **Reopen Window Minutes**, else `AlertsQBitConfig.withReopenWindowMinutesByTypePrefix` (default `health.` = 360 minutes), else `withReopenWindowMinutes` (default 30). A health check that blips again within 6 hours reopens the same alert and bell entry instead of creating a new one. ## Unreleased: starter defaults, setup card, table help diff --git a/src/main/java/com/kingsrook/qbits/alerts/AlertsQBitProducer.java b/src/main/java/com/kingsrook/qbits/alerts/AlertsQBitProducer.java index cff8d5e..dfcacdb 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/AlertsQBitProducer.java +++ b/src/main/java/com/kingsrook/qbits/alerts/AlertsQBitProducer.java @@ -297,9 +297,9 @@ public class AlertsQBitProducer implements QBitMetaDataProducer queue = new ArrayBlockingQueue<>(QUEUE_CAPACITY); @@ -67,6 +73,7 @@ public class QbitAlertsSink implements AlertSinkInterface private final AtomicLong dropped = new AtomicLong(); private final Map perMinute = new ConcurrentHashMap<>(); private volatile long minute = -1; + private volatile long drainedSinceMillis = -1; private volatile Thread drainThread; private volatile boolean stopping = false; @@ -237,14 +244,87 @@ public class QbitAlertsSink implements AlertSinkInterface if(!queue.offer(runnable)) { dropped.incrementAndGet(); - if(overflowRaised.compareAndSet(false, true)) - { - LOG.warn("The alerts queue is full; dropping signals", logPair("capacity", QUEUE_CAPACITY)); - Thread.ofVirtual().start(() -> safely(() -> runtime.getEngine().raise(new AlertSignal(OVERFLOW_TYPE) - .withSeverity(AlertSeverity.ERROR) - .withTitle("Alert signals are being dropped") - .withSummary("The alerts queue was full, so some signals were not recorded.")))); - } + overflowed(); + } + } + + + + /******************************************************************************* + ** A signal was dropped because the queue is full: raise the overflow alert + ** (once, until the queue has drained). + *******************************************************************************/ + void overflowed() + { + drainedSinceMillis = -1; + if(overflowRaised.compareAndSet(false, true)) + { + LOG.warn("The alerts queue is full; dropping signals", logPair("capacity", QUEUE_CAPACITY)); + runMetaAlert(() -> runtime.getEngine().raise(new AlertSignal(OVERFLOW_TYPE) + .withDedupKey(OVERFLOW_DEDUP_KEY) + .withSeverity(AlertSeverity.ERROR) + .withTitle("Alert signals are being dropped") + .withSummary("The alerts queue was full, so some signals were not recorded."))); + } + } + + + + /******************************************************************************* + ** Called by the drain thread: once the overflow alert is raised and the + ** queue has stayed below half full for OVERFLOW_CLEAR_MILLIS, resolve the + ** alert and re-arm it, so a later overflow raises it again. + *******************************************************************************/ + void checkOverflowCleared() + { + if(!overflowRaised.get()) + { + return; + } + long now = runtime.now().toEpochMilli(); + if(queue.size() >= QUEUE_CAPACITY / 2) + { + drainedSinceMillis = -1; + return; + } + if(drainedSinceMillis < 0) + { + drainedSinceMillis = now; + return; + } + if(now - drainedSinceMillis >= OVERFLOW_CLEAR_MILLIS && overflowRaised.compareAndSet(true, false)) + { + drainedSinceMillis = -1; + LOG.info("The alerts queue has drained; resolving the overflow alert", logPair("dropped", dropped.get())); + runMetaAlert(() -> runtime.getEngine().resolve(OVERFLOW_TYPE, OVERFLOW_DEDUP_KEY, Resolution.PRODUCER, null, + "The queue stayed below half full for " + (OVERFLOW_CLEAR_MILLIS / 1000) + " seconds.")); + } + } + + + + /******************************************************************************* + ** Whether the overflow alert is raised and not yet cleared. + *******************************************************************************/ + boolean isOverflowRaised() + { + return (overflowRaised.get()); + } + + + + /******************************************************************************* + ** Record a meta-alert off the producer's thread (on it with synchronousSink). + *******************************************************************************/ + private void runMetaAlert(EngineWork work) + { + if(Boolean.TRUE.equals(runtime.getConfig().getSynchronousSink())) + { + safely(work); + } + else + { + Thread.ofVirtual().start(() -> safely(work)); } } @@ -296,6 +376,7 @@ public class QbitAlertsSink implements AlertSinkInterface { work.run(); } + checkOverflowCleared(); } catch(InterruptedException e) { diff --git a/src/test/java/com/kingsrook/qbits/alerts/engine/SinkOverflowTest.java b/src/test/java/com/kingsrook/qbits/alerts/engine/SinkOverflowTest.java new file mode 100644 index 0000000..480c401 --- /dev/null +++ b/src/test/java/com/kingsrook/qbits/alerts/engine/SinkOverflowTest.java @@ -0,0 +1,92 @@ +/* + * Copyright 2026 Kingsrook, LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.kingsrook.qbits.alerts.engine; + + +import java.time.Duration; +import java.util.List; +import com.kingsrook.qbits.alerts.BaseAlertsTest; +import com.kingsrook.qbits.alerts.model.AlertsAlert; +import com.kingsrook.qbits.alerts.model.AlertsPossibleValues.Status; +import com.kingsrook.qqq.backend.core.model.data.QRecord; +import org.junit.jupiter.api.Test; +import static org.assertj.core.api.Assertions.assertThat; + + +/******************************************************************************* + ** The sink's overflow alert: raised once with a stable key, resolved by the + ** drain thread once the queue has stayed below half full for a minute, and + ** raised again by a later overflow. + *******************************************************************************/ +class SinkOverflowTest extends BaseAlertsTest +{ + + /******************************************************************************* + ** Raise once, clear after a sustained drain, raise again. + *******************************************************************************/ + @Test + void testOverflowRaisesOnceAndClears() throws Exception + { + QbitAlertsSink sink = runtime.getSink(); + sink.checkOverflowCleared(); + assertThat(overflowAlerts()).isEmpty(); + + sink.overflowed(); + sink.overflowed(); + List alerts = overflowAlerts(); + assertThat(alerts).hasSize(1); + QRecord alert = alerts.get(0); + assertThat(alert.getValueString("dedupKey")).isEqualTo(QbitAlertsSink.OVERFLOW_DEDUP_KEY); + assertThat(alert.getValueString("severity")).isEqualTo("ERROR"); + assertThat(alert.getValueString("status")).isEqualTo(Status.OPEN.name()); + assertThat(sink.isOverflowRaised()).isTrue(); + + ////////////////////////////////////////////////////////////////// + // the drain must be sustained: a new overflow restarts the wait // + ////////////////////////////////////////////////////////////////// + sink.checkOverflowCleared(); + clock.advance(Duration.ofSeconds(45)); + sink.overflowed(); + sink.checkOverflowCleared(); + clock.advance(Duration.ofSeconds(45)); + sink.checkOverflowCleared(); + assertThat(overflowAlerts().get(0).getValueString("status")).isEqualTo(Status.OPEN.name()); + + clock.advance(Duration.ofSeconds(20)); + sink.checkOverflowCleared(); + assertThat(overflowAlerts().get(0).getValueString("status")).isEqualTo(Status.RESOLVED.name()); + assertThat(sink.isOverflowRaised()).isFalse(); + + //////////////////////////////////////////// + // a later overflow raises it again // + //////////////////////////////////////////// + clock.advance(Duration.ofHours(2)); + sink.overflowed(); + assertThat(overflowAlerts()).filteredOn(a -> Status.OPEN.name().equals(a.getValueString("status"))).hasSize(1); + assertThat(sink.isOverflowRaised()).isTrue(); + } + + + + /******************************************************************************* + ** + *******************************************************************************/ + private List overflowAlerts() throws Exception + { + return (where(AlertsAlert.TABLE_NAME, AlertsDb.eq("alertTypeName", QbitAlertsSink.OVERFLOW_TYPE))); + } +} -- 2.54.0 From 58195c43abfad21d0d4e83bd72dac30a945022ab Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 16 Oct 2026 15:49:54 -0500 Subject: [PATCH 4/5] fix(notifications): drop the "API key created" notice It told people about an API key they had just created. API key expiry, access-change and share notices stay. newSignIn is unchanged: the qbit keeps no sign-in history, so the host decides when a device or address is new; its Javadoc and the docs now say so. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +++ README.md | 2 +- UPGRADE.md | 1 + docs/NOTIFICATIONS.md | 4 ++-- .../alerts/notifications/AlertsNotices.java | 18 ++++-------------- .../NotificationEmailAndNoticesTest.java | 8 ++++---- 6 files changed, 15 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a09f557..2aa2f19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ All notable changes to this project are documented here. The format follows Keep ## [0.1.0] - Unreleased +### Removed +- `AlertsNotices.apiKeyCreated`: it told people about an API key they had just created. API key expiry, access-change and share notices stay. `newSignIn` is unchanged (the qbit has no sign-in history to tell a new device or address from a known one); its Javadoc now says hosts call it only for an unfamiliar device or address. + ### Fixed - "Alert signals are being dropped" (`alerts.sink.overflow`) stayed open forever: it had no dedup key, was manual-resolve, and the sink never re-armed it. It now has a stable dedup key, and the drain thread resolves it once the queue has stayed below half full for a minute, so a later overflow raises it again. Still ERROR, admins only. - Duplicate health alerts: a health check that failed again more than 30 minutes after its last resolve created a second alert and a second bell entry for the same problem. The reopen window is now per type: `alerts_type.reopenWindowMinutes` (changeset 55), else the longest matching prefix in `AlertsQBitConfig.reopenWindowMinutesByTypePrefix` (default `health.` = 360 minutes), else `reopenWindowMinutes` (30). A reopen updates the existing alert and its bell entry; flapping detection is unchanged. diff --git a/README.md b/README.md index 8b1086f..d93887e 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ The qbit also carries ordinary notifications for people (design: [docs/NOTIFICAT - **Admins** with `alerts.notification.admin` write notifications in the Notifications table (title, markdown body, severity, category, link, audience, publish and expire times, a sticky banner flag), then use the record actions Preview audience, Send now, Schedule and Retract. Edits after sending show at once; retracting hides it. - **Audiences** are resolved when sent: all users, roles or groups, holders of a permission, or named users. Each user gets one `alerts_notification_recipient` row, so read, unread and dismissed are per user. Roles need a user directory (`AlertsQBitConfig.withUserDirectory`, an `AlertsUserDirectoryInterface`); without one, "all users" means everyone who has used Alerts. - **Producers** send with `QAlerts.notify(new NotificationSignal(...))` using `com.kingsrook.qqq.backend.core.alerts` types (RC.6). Alerts a user subscribes to arrive in the bell as ALERT notifications. -- **Ready-made notices** in `AlertsNotices`: `newSignIn`, `apiKeyCreated`, `apiKeyExpiring`, `permissionChanged` (SECURITY), `shared` (SHARE) and `release` (RELEASE, all users). Core has no sign-in or API key events yet, so hosts call these from their authentication customizer and API key screens. +- **Ready-made notices** in `AlertsNotices`: `newSignIn` (call it only for a device or address not seen recently), `apiKeyExpiring`, `permissionChanged` (SECURITY), `shared` (SHARE) and `release` (RELEASE, all users). Core has no sign-in or API key events yet, so hosts call these from their authentication customizer and API key screens. - **Email** is opt-in per user and category (IMMEDIATE, or DAILY_DIGEST at the digest hour in My Alert Settings), through the configured messaging provider; without one it is off. ALERT notifications are not emailed here (alert subscriptions already do), and SECURITY emails carry only the title and link. - **Failed and partial jobs** notify the person who started them (`notifyFailedJobs`, `jobRunLinkPattern`). - **Links** must be app paths with one leading slash, or https URLs; backslashes, whitespace and control characters are refused. diff --git a/UPGRADE.md b/UPGRADE.md index 29bba26..71bc307 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -6,6 +6,7 @@ 2. **Behavior changes to know about.** - A resolve no longer marks the alert's bell entry unread; the entry is updated in place ("Resolved: ..."). A subscription whose events name RESOLVED and differ from the default set keeps the old behavior. Email, Slack and webhook resolve notices are unchanged. - "Alert signals dropped" (`alerts.sink.overflow`) now resolves itself once the queue has stayed below half full for a minute. An installed type row keeps its stored Resolve Mode label; the behavior does not depend on it. + - `AlertsNotices.apiKeyCreated` is removed. Delete the call; there is no replacement notice. - The reopen window is now per alert type: an alert type's **Reopen Window Minutes**, else `AlertsQBitConfig.withReopenWindowMinutesByTypePrefix` (default `health.` = 360 minutes), else `withReopenWindowMinutes` (default 30). A health check that blips again within 6 hours reopens the same alert and bell entry instead of creating a new one. ## Unreleased: starter defaults, setup card, table help diff --git a/docs/NOTIFICATIONS.md b/docs/NOTIFICATIONS.md index 7e29107..e0fa759 100644 --- a/docs/NOTIFICATIONS.md +++ b/docs/NOTIFICATIONS.md @@ -33,7 +33,7 @@ system as well" - their failed or partial jobs (processes, ETL, imports and exports they started), linked to the run; - alerts they subscribe to; - things assigned or shared to them (a notebook share, a record lock, a pending approval); - - security notices (new sign-in, API key created or expiring, permission change); + - security notices (new sign-in from an unfamiliar device or address, API key expiring, permission change); - application releases and marketing notifications (authored by admins or emitted by apps, such as "new version released" or a marketing-qbit campaign), sent to an audience. @@ -235,7 +235,7 @@ QAlerts.notify(new NotificationSignal("Your export failed") | Failed or partial jobs | The qbit's process lifecycle listener: a run that fails, or completes with error summary lines, notifies the user who started it (the session user; system runs are skipped). The link is the run's table, or `jobRunLinkPattern` with `{processName}` and `{processUUID}`. Turn it off with `notifyFailedJobs(false)`. | JOB | | Alerts they subscribe to | The router's in-app channel writes an ALERT notification per alert (sourceKey `alert:`) and a recipient row per routed user. A raise, reopen, severity increase, reminder or escalation marks it unread again. A resolve only updates it in place (title "Resolved: ...", read state unchanged), unless the subscription chose resolve notices on purpose (its events name RESOLVED and differ from the default set). | ALERT | | Shares, record locks, approvals | Producers call `QAlerts.notify(... user(id))`. The first wiring is the SQL Workbench notebook share in kof22. | SHARE | -| Security notices | `AlertsSecurityNotices` (new sign-in, API key created, API key expiring, permission change). Core has no authentication events today, so hosts call it from their authentication customizer. A core hook is follow-up work (section 12, Q5). Security is never mutable in-app. | SECURITY | +| Security notices | `AlertsSecurityNotices` (new sign-in, API key expiring, permission change). Core has no authentication events today, so hosts call it from their authentication customizer; the host decides whether a sign-in is from a new device or address, since the qbit keeps no sign-in history. Creating an API key sends nothing: the person who created it already knows. A core hook is follow-up work (section 12, Q5). Security is never mutable in-app. | SECURITY | | Releases and marketing | Admin-authored notifications, or producers with an audience | RELEASE, MARKETING | ## 7. Delivery diff --git a/src/main/java/com/kingsrook/qbits/alerts/notifications/AlertsNotices.java b/src/main/java/com/kingsrook/qbits/alerts/notifications/AlertsNotices.java index b6ab47f..66b3716 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/notifications/AlertsNotices.java +++ b/src/main/java/com/kingsrook/qbits/alerts/notifications/AlertsNotices.java @@ -37,7 +37,8 @@ import com.kingsrook.qqq.backend.core.alerts.QAlerts; ** qbit installed core uses its logging sink. ** ** Core has no sign-in or API key events yet, so a host calls the security - ** notices from its authentication customizer or API key screens. + ** notices from its authentication customizer or API key screens. There is + ** no notice for creating an API key: the person who created it just did it. *******************************************************************************/ public final class AlertsNotices { @@ -56,7 +57,8 @@ public final class AlertsNotices /******************************************************************************* ** A sign-in from a new device or place (where: e.g. "Firefox on Windows, - ** near Columbus, OH"). + ** near Columbus, OH"). Call it only for a device or address not seen for + ** this user recently: the host has the sign-in history, this qbit does not. *******************************************************************************/ public static void newSignIn(String userId, String where) { @@ -67,18 +69,6 @@ public final class AlertsNotices - /******************************************************************************* - ** An API key was created for the user. - *******************************************************************************/ - public static void apiKeyCreated(String userId, String keyLabel) - { - send(new NotificationSignal("API key created: " + keyLabel) - .withBody("A new API key was created on your account. If you didn't create it, revoke it now.") - .withSeverity(NotificationSeverity.INFO), userId, null); - } - - - /******************************************************************************* ** An API key expires soon (one notification per key, refreshed on repeat). *******************************************************************************/ diff --git a/src/test/java/com/kingsrook/qbits/alerts/notifications/NotificationEmailAndNoticesTest.java b/src/test/java/com/kingsrook/qbits/alerts/notifications/NotificationEmailAndNoticesTest.java index 0b59e98..37cbf46 100644 --- a/src/test/java/com/kingsrook/qbits/alerts/notifications/NotificationEmailAndNoticesTest.java +++ b/src/test/java/com/kingsrook/qbits/alerts/notifications/NotificationEmailAndNoticesTest.java @@ -434,19 +434,19 @@ class NotificationEmailAndNoticesTest extends BaseAlertsTest emailSettings("viewer-1", "SECURITY", "IMMEDIATE", "INFO"); loginAs("viewer-1"); AlertsNotices.newSignIn("viewer-1", "Firefox on Windows"); - AlertsNotices.apiKeyCreated("viewer-1", "ci-bot"); AlertsNotices.apiKeyExpiring("viewer-1", "ci-bot", Instant.parse("2026-11-01T00:00:00Z")); AlertsNotices.apiKeyExpiring("viewer-1", "ci-bot", Instant.parse("2026-11-01T00:00:00Z")); AlertsNotices.permissionChanged("viewer-1", "You were added to the Editors role"); AlertsNotices.shared("viewer-1", "Sam shared a notebook with you", AlertLink.toPath("/app/widget", "Open"), "qbit-sql-workbench"); List notifications = runtime.asSystem(() -> all(AlertsNotification.TABLE_NAME)); - assertThat(notifications).hasSize(5); - assertThat(notifications).filteredOn(n -> "SECURITY".equals(n.getValueString("category"))).hasSize(4); + assertThat(notifications).hasSize(4); + assertThat(notifications).filteredOn(n -> "SECURITY".equals(n.getValueString("category"))).hasSize(3); assertThat(notifications).anyMatch(n -> n.getValueString("title").equals("API key expires Nov 1, 2026: ci-bot")); + assertThat(notifications).noneMatch(n -> n.getValueString("title").startsWith("API key created")); new NotificationEmailer(runtime).run(); - assertThat(SENT).hasSize(4); + assertThat(SENT).hasSize(3); assertThat(SENT).noneMatch(message -> text(message).contains("change your password")); } -- 2.54.0 From ee5398b2ea638d882c77803b3965b7cf74c88bdd Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 16 Oct 2026 15:51:54 -0500 Subject: [PATCH 5/5] fix(notifications): job notices only for runs nobody was watching " failed" and " finished with errors" went to whoever started any run, including interactive runs whose result screen already showed the outcome. The listener now notifies only for runs that skip or fail on frontend steps (scheduled, background, API, queue runs); a run on the process screens (FrontendStepBehavior.BREAK) sends nothing. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + README.md | 2 +- UPGRADE.md | 1 + docs/NOTIFICATIONS.md | 2 +- .../alerts/engine/AlertsProcessListener.java | 26 +++++++++++++++-- .../notifications/JobNotificationsTest.java | 29 +++++++++++++++++++ 6 files changed, 56 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2aa2f19..dd0fbb2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ All notable changes to this project are documented here. The format follows Keep - `AlertsNotices.apiKeyCreated`: it told people about an API key they had just created. API key expiry, access-change and share notices stay. `newSignIn` is unchanged (the qbit has no sign-in history to tell a new device or address from a known one); its Javadoc now says hosts call it only for an unfamiliar device or address. ### Fixed +- Job notifications for runs the person was watching: " failed" and " finished with errors" went to whoever started any run, including interactive runs whose result screen already showed it. Only runs with `FrontendStepBehavior` SKIP or FAIL (scheduled, background, API, queue) notify now. - "Alert signals are being dropped" (`alerts.sink.overflow`) stayed open forever: it had no dedup key, was manual-resolve, and the sink never re-armed it. It now has a stable dedup key, and the drain thread resolves it once the queue has stayed below half full for a minute, so a later overflow raises it again. Still ERROR, admins only. - Duplicate health alerts: a health check that failed again more than 30 minutes after its last resolve created a second alert and a second bell entry for the same problem. The reopen window is now per type: `alerts_type.reopenWindowMinutes` (changeset 55), else the longest matching prefix in `AlertsQBitConfig.reopenWindowMinutesByTypePrefix` (default `health.` = 360 minutes), else `reopenWindowMinutes` (30). A reopen updates the existing alert and its bell entry; flapping detection is unchanged. - Bell noise: a resolve marked the alert's bell entry unread again. It now updates the entry in place (title "Resolved: ...") and leaves the read state alone. A subscription whose events name RESOLVED and differ from the default set (`AlertRouter.DEFAULT_SUBSCRIPTION_EVENTS`) keeps the old behavior. External channels are unchanged. diff --git a/README.md b/README.md index d93887e..e2bf168 100644 --- a/README.md +++ b/README.md @@ -74,7 +74,7 @@ The qbit also carries ordinary notifications for people (design: [docs/NOTIFICAT - **Producers** send with `QAlerts.notify(new NotificationSignal(...))` using `com.kingsrook.qqq.backend.core.alerts` types (RC.6). Alerts a user subscribes to arrive in the bell as ALERT notifications. - **Ready-made notices** in `AlertsNotices`: `newSignIn` (call it only for a device or address not seen recently), `apiKeyExpiring`, `permissionChanged` (SECURITY), `shared` (SHARE) and `release` (RELEASE, all users). Core has no sign-in or API key events yet, so hosts call these from their authentication customizer and API key screens. - **Email** is opt-in per user and category (IMMEDIATE, or DAILY_DIGEST at the digest hour in My Alert Settings), through the configured messaging provider; without one it is off. ALERT notifications are not emailed here (alert subscriptions already do), and SECURITY emails carry only the title and link. -- **Failed and partial jobs** notify the person who started them (`notifyFailedJobs`, `jobRunLinkPattern`). +- **Failed and partial jobs** notify the person who started them when the run was not interactive (scheduled, background or API runs; `notifyFailedJobs`, `jobRunLinkPattern`). - **Links** must be app paths with one leading slash, or https URLs; backslashes, whitespace and control characters are refused. - **Preferences:** the Notifications section combines pause, digest time and zone, alert switches, per-category in-app/email/Slack settings, and the user's own subscriptions. It uses existing tables and FIELD audits; SECURITY cannot be muted in-app. Slack controls appear when the configured per-user Slack provider is registered before Alerts is produced; SECURITY Slack settings stay editable. Uses core's released user-preferences contract in QQQ `4.1.0-RC.7`; disable with `withPreferencesSectionEnabled(false)`. See [Notification settings in Preferences](docs/NOTIFICATIONS.md#34-notification-settings-in-preferences). - **People** see them in the Next UI bell and notifications page, and choose settings in Preferences > Notifications or the standard My Notification Settings table (security notices can't be muted). diff --git a/UPGRADE.md b/UPGRADE.md index 71bc307..04634eb 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -7,6 +7,7 @@ - A resolve no longer marks the alert's bell entry unread; the entry is updated in place ("Resolved: ..."). A subscription whose events name RESOLVED and differ from the default set keeps the old behavior. Email, Slack and webhook resolve notices are unchanged. - "Alert signals dropped" (`alerts.sink.overflow`) now resolves itself once the queue has stayed below half full for a minute. An installed type row keeps its stored Resolve Mode label; the behavior does not depend on it. - `AlertsNotices.apiKeyCreated` is removed. Delete the call; there is no replacement notice. + - Failed and partial job notifications are sent only for runs that were not interactive (frontend steps skipped: scheduled, background, API). A run on the process screens shows its result there instead. - The reopen window is now per alert type: an alert type's **Reopen Window Minutes**, else `AlertsQBitConfig.withReopenWindowMinutesByTypePrefix` (default `health.` = 360 minutes), else `withReopenWindowMinutes` (default 30). A health check that blips again within 6 hours reopens the same alert and bell entry instead of creating a new one. ## Unreleased: starter defaults, setup card, table help diff --git a/docs/NOTIFICATIONS.md b/docs/NOTIFICATIONS.md index e0fa759..bd47dc5 100644 --- a/docs/NOTIFICATIONS.md +++ b/docs/NOTIFICATIONS.md @@ -232,7 +232,7 @@ QAlerts.notify(new NotificationSignal("Your export failed") | Source | How | Category | |---|---|---| -| Failed or partial jobs | The qbit's process lifecycle listener: a run that fails, or completes with error summary lines, notifies the user who started it (the session user; system runs are skipped). The link is the run's table, or `jobRunLinkPattern` with `{processName}` and `{processUUID}`. Turn it off with `notifyFailedJobs(false)`. | JOB | +| Failed or partial jobs | The qbit's process lifecycle listener: a run that fails, or completes with error summary lines, notifies the user who started it (the session user; system runs are skipped). Only runs nobody is watching notify (scheduled, background, API: frontend steps skipped); an interactive run on the process screens already shows its result. The link is the run's table, or `jobRunLinkPattern` with `{processName}` and `{processUUID}`. Turn it off with `notifyFailedJobs(false)`. | JOB | | Alerts they subscribe to | The router's in-app channel writes an ALERT notification per alert (sourceKey `alert:`) and a recipient row per routed user. A raise, reopen, severity increase, reminder or escalation marks it unread again. A resolve only updates it in place (title "Resolved: ...", read state unchanged), unless the subscription chose resolve notices on purpose (its events name RESOLVED and differ from the default set). | ALERT | | Shares, record locks, approvals | Producers call `QAlerts.notify(... user(id))`. The first wiring is the SQL Workbench notebook share in kof22. | SHARE | | Security notices | `AlertsSecurityNotices` (new sign-in, API key expiring, permission change). Core has no authentication events today, so hosts call it from their authentication customizer; the host decides whether a sign-in is from a new device or address, since the qbit keeps no sign-in history. Creating an API key sends nothing: the person who created it already knows. A core hook is follow-up work (section 12, Q5). Security is never mutable in-app. | SECURITY | diff --git a/src/main/java/com/kingsrook/qbits/alerts/engine/AlertsProcessListener.java b/src/main/java/com/kingsrook/qbits/alerts/engine/AlertsProcessListener.java index b6500c5..1141eea 100644 --- a/src/main/java/com/kingsrook/qbits/alerts/engine/AlertsProcessListener.java +++ b/src/main/java/com/kingsrook/qbits/alerts/engine/AlertsProcessListener.java @@ -60,8 +60,12 @@ import static com.kingsrook.qqq.backend.core.logging.LogUtils.logPair; ** cached for a minute). ** - Job notifications (docs/NOTIFICATIONS.md 6.1): when a run started by a ** person fails, or completes with error summary lines (a partial run), - ** that person gets a JOB notification linked to the run. System runs and - ** the qbit's own processes are skipped. Off with notifyFailedJobs(false). + ** that person gets a JOB notification linked to the run. Only runs nobody + ** is watching notify: scheduled, background, API and other runs that skip + ** frontend steps. An interactive run (FrontendStepBehavior.BREAK, the + ** process screens) already shows its result, so it sends nothing. System + ** runs and the qbit's own processes are skipped. Off with + ** notifyFailedJobs(false). *******************************************************************************/ public class AlertsProcessListener implements ProcessLifecycleListenerInterface { @@ -230,10 +234,14 @@ public class AlertsProcessListener implements ProcessLifecycleListenerInterface /******************************************************************************* ** Send a JOB notification to the person who started the run (nothing for - ** system runs or sessions without a user). + ** interactive runs, system runs or sessions without a user). *******************************************************************************/ private static void notifyStarter(AlertsRuntime runtime, RunProcessInput input, NotificationSeverity severity, String titleFormat, String body) { + if(isInteractive(input)) + { + return; + } QSession session = QContext.getQSession(); String userId = session == null || session instanceof QSystemUserSession ? null : AlertsSecurity.userId(session); if(userId == null) @@ -256,6 +264,18 @@ public class AlertsProcessListener implements ProcessLifecycleListenerInterface + /******************************************************************************* + ** Whether a person is watching the run on the process screens: they stop + ** at frontend steps (BREAK) and end on the result screen. Scheduled, + ** background, API and queue runs skip or fail on frontend steps. + *******************************************************************************/ + public static boolean isInteractive(RunProcessInput input) + { + return (input != null && input.getFrontendStepBehavior() == RunProcessInput.FrontendStepBehavior.BREAK); + } + + + /******************************************************************************* ** Where a job notification links: the configured run link pattern, else ** the process's table, else nowhere. diff --git a/src/test/java/com/kingsrook/qbits/alerts/notifications/JobNotificationsTest.java b/src/test/java/com/kingsrook/qbits/alerts/notifications/JobNotificationsTest.java index 1a7ef11..ca808f8 100644 --- a/src/test/java/com/kingsrook/qbits/alerts/notifications/JobNotificationsTest.java +++ b/src/test/java/com/kingsrook/qbits/alerts/notifications/JobNotificationsTest.java @@ -124,6 +124,35 @@ class JobNotificationsTest extends BaseAlertsTest + /******************************************************************************* + ** A run the person watches on the process screens (BREAK) already shows + ** its result, so it sends nothing. + *******************************************************************************/ + @Test + void testInteractiveRunNotifiesNobody() throws Exception + { + TestSteps.fail = true; + loginAs("viewer-1"); + try + { + new RunProcessAction().execute(new RunProcessInput().withProcessName("nightlyThing").withFrontendStepBehavior(RunProcessInput.FrontendStepBehavior.BREAK)); + } + catch(Exception e) + { + //////////////////////////////////////// + // the run's own failure is expected // + //////////////////////////////////////// + } + assertThat(runtime.asSystem(() -> all(AlertsNotification.TABLE_NAME))).isEmpty(); + + assertThat(AlertsProcessListener.isInteractive(null)).isFalse(); + assertThat(AlertsProcessListener.isInteractive(new RunProcessInput())).isTrue(); + assertThat(AlertsProcessListener.isInteractive(new RunProcessInput().withFrontendStepBehavior(RunProcessInput.FrontendStepBehavior.SKIP))).isFalse(); + assertThat(AlertsProcessListener.isInteractive(new RunProcessInput().withFrontendStepBehavior(RunProcessInput.FrontendStepBehavior.FAIL))).isFalse(); + } + + + /******************************************************************************* ** Turned off, nothing is sent. *******************************************************************************/ -- 2.54.0