From 84cdddb1766011422e94355d9f4da69512115983 Mon Sep 17 00:00:00 2001 From: Johannes Schmidt Date: Wed, 22 Oct 2025 10:16:59 +0200 Subject: [PATCH 1/4] Add ClearTestLogger method to TestLoggerFixture --- test/base-testloggerfixture.hpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/base-testloggerfixture.hpp b/test/base-testloggerfixture.hpp index 69c073b02a..fd20386783 100644 --- a/test/base-testloggerfixture.hpp +++ b/test/base-testloggerfixture.hpp @@ -52,6 +52,13 @@ class TestLogger : public Logger return ret; } + void Clear() + { + std::lock_guard lock(m_Mutex); + m_Expects.clear(); + m_LogEntries.clear(); + } + private: void ProcessLogEntry(const LogEntry& entry) override { @@ -119,6 +126,8 @@ struct TestLoggerFixture return testLogger->ExpectLogPattern(pattern, timeout); } + void ClearTestLogger() const { testLogger->Clear(); } + TestLogger::Ptr testLogger = new TestLogger; }; From 68b3b9fd3a87a306471da82eae73eddbcab959b8 Mon Sep 17 00:00:00 2001 From: Johannes Schmidt Date: Mon, 20 Oct 2025 09:39:48 +0200 Subject: [PATCH 2/4] Add unit-tests for NotificationComponent This includes a few common scenarios and a reproduction of the current behavior affected by the underlying bug of issue #10575. This is done both to document the change in behavior, as well as to ensure the behavior of the other scenarios stays the same before and after the fix is applied. --- test/CMakeLists.txt | 7 + test/notification-notificationcomponent.cpp | 506 ++++++++++++++++++++ 2 files changed, 513 insertions(+) create mode 100644 test/notification-notificationcomponent.cpp diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index fc9edac2bd..0bf4b84133 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -131,6 +131,13 @@ set(base_test_SOURCES $ ) +if(ICINGA2_WITH_NOTIFICATION) + list(APPEND base_test_SOURCES + notification-notificationcomponent.cpp + $ + ) +endif() + if(ICINGA2_UNITY_BUILD) mkunity_target(base test base_test_SOURCES) endif() diff --git a/test/notification-notificationcomponent.cpp b/test/notification-notificationcomponent.cpp new file mode 100644 index 0000000000..880ef92525 --- /dev/null +++ b/test/notification-notificationcomponent.cpp @@ -0,0 +1,506 @@ +/* Icinga 2 | (c) 2025 Icinga GmbH | GPLv2+ */ + +#include +#include "base/defer.hpp" +#include "remote/apilistener.hpp" +#include "test/base-testloggerfixture.hpp" +#include "config/configcompiler.hpp" +#include "notification/notificationcomponent.hpp" + +using namespace icinga; + +namespace { + +/** + * Gets the pointer to the private NotificationTimerHandler() by using Friend-Injection. + * + * This uses the exception the standard makes to private member access for explicit template + * instantiation by instantiating a type that defines an accessor to the member function pointer + * in the surrounding anonymous namespace. + * + * The reason for the anonymous namespace is that it doesn't violate the ODR if other + * instantiations are made to this template in other translation units. + * This isn't actually an issue here because the name is very specific to the single use-case of + * obtaining access to NotificationTimerHandler(). + */ +template +struct InvokeTimerHandlerImpl +{ + friend void InvokeTimerHandler(const NotificationComponent::Ptr& nc) { (*nc.*privateMemberFnPtr)(); } +}; +void InvokeTimerHandler(const NotificationComponent::Ptr& nc); + +template struct InvokeTimerHandlerImpl<&NotificationComponent::NotificationTimerHandler>; + +} // namespace + +class NotificationComponentFixture : public TestLoggerFixture +{ +public: + NotificationComponentFixture() + { + auto createObjects = []() { + String config = R"CONFIG({ +object CheckCommand "dummy" { + command = "/bin/echo" +} +object Host "h1" { + address = "h1" + check_command = "dummy" + enable_notifications = true + enable_active_checks = false + enable_passive_checks = true +} +object NotificationCommand "send" { + command = ["true"] +} +apply Notification "n1" to Host { + interval = 0 + command = "send" + period = "tp1" + users = [ "u1" ] + assign where host.enable_notifications == true +} +object User "u1" { + enable_notifications = true +} +object TimePeriod "tp1" { + display_name = "Test TimePeriod" + ranges = { + "monday" = "00:00-24:00" + "tuesday" = "00:00-24:00" + "wednesday" = "00:00-24:00" + "thursday" = "00:00-24:00" + "friday" = "00:00-24:00" + "saturday" = "00:00-24:00" + "sunday" = "00:00-24:00" + } +} +object NotificationComponent "nc" {} +})CONFIG"; + std::unique_ptr expr = ConfigCompiler::CompileText("", config); + expr->Evaluate(*ScriptFrame::GetCurrentFrame()); + }; + + auto ret = ConfigItem::RunWithActivationContext(new Function("CreateTestObjects", createObjects)); + BOOST_REQUIRE(ret); + + m_Host = Host::GetByName("h1"); + BOOST_REQUIRE(m_Host); + + m_Notification = Notification::GetByName("h1!n1"); + BOOST_REQUIRE(m_Notification); + + m_TimePeriod = TimePeriod::GetByName("tp1"); + BOOST_REQUIRE(m_TimePeriod); + + ApiListener::UpdateObjectAuthority(); + BOOST_REQUIRE(ApiListener::UpdatedObjectAuthority()); + + // Store the old periods from the config snippets to reuse them later. + m_AllTimePeriod = m_TimePeriod->GetRanges(); + + Checkable::OnNotificationSentToUser.connect(NotificationSentToUserHandler); + } + + static void NotificationSentToUserHandler( + const Notification::Ptr&, + const Checkable::Ptr&, + const User::Ptr&, + const NotificationType& type, + const CheckResult::Ptr&, + const String&, + const String&, + const String&, + const MessageOrigin::Ptr& + ) + { + std::unique_lock lock(m_NotificationMutex); + m_LastNotification = type; + m_NumNotifications++; + lock.unlock(); + m_NotificationCv.notify_all(); + } + + bool WaitForExpectedNotificationCount(std::size_t expectedCount, std::chrono::milliseconds timeout = 5s) + { + Defer clearLog{[this]() { ClearTestLogger(); }}; + std::unique_lock lock(m_NotificationMutex); + if (m_NumNotifications > expectedCount) { + return false; + } + return m_NotificationCv.wait_for(lock, timeout, [&]() { return m_NumNotifications == expectedCount; }); + } + + boost::test_tools::assertion_result AssertNoAttemptedSendLogPattern() + { + auto result = ExpectLogPattern("^(Sending|Attempting to (re-)?send).*?notification.*$", 0s); + ClearTestLogger(); + return !result; + } + + void BeginTimePeriod() + { + ObjectLock lock{m_TimePeriod}; + m_TimePeriod->SetRanges(m_AllTimePeriod); + + auto now = Utility::GetTime(); + m_TimePeriod->UpdateRegion(now, now + 1e3, true); + BOOST_REQUIRE(m_TimePeriod->IsInside(now)); + } + + void EndTimePeriod() + { + ObjectLock lock{m_TimePeriod}; + m_TimePeriod->SetRanges(new Dictionary); + + auto now = Utility::GetTime(); + m_TimePeriod->UpdateRegion(now, now + 1e3, true); + BOOST_REQUIRE(!m_TimePeriod->IsInside(now)); + } + + void SetNotificationInverval(double interval) { m_Notification->SetInterval(interval); } + + void WaitUntilNextReminderScheduled() + { + Utility::Sleep(m_Notification->GetNextNotification() - Utility::GetTime() + 0.01); + BOOST_REQUIRE_LE(m_Notification->GetNextNotification(), Utility::GetTime()); + } + + static void NotificationTimerHandler() + { + auto nc = NotificationComponent::GetByName("nc"); + InvokeTimerHandler(nc); + } + + void ReceiveCheckResults(std::size_t num, ServiceState state) + { + StoppableWaitGroup::Ptr wg = new StoppableWaitGroup(); + + for (auto i = 0UL; i < num; ++i) { + CheckResult::Ptr cr = new CheckResult(); + + cr->SetState(state); + + double now = Utility::GetTime(); + cr->SetActive(false); + cr->SetScheduleStart(now); + cr->SetScheduleEnd(now); + cr->SetExecutionStart(now); + cr->SetExecutionEnd(now); + + BOOST_REQUIRE(m_Host->ProcessCheckResult(cr, wg) == Checkable::ProcessingResult::Ok); + } + } + + double GetLastNotificationTimestamp() { return m_Notification->GetLastNotification(); } + std::uint8_t GetSuppressedNotifications() { return m_Notification->GetSuppressedNotifications(); } + static NotificationType GetLastNotification() { return m_LastNotification; } + static std::size_t GetNotificationCount() { return m_NumNotifications; } + +private: + static inline std::mutex m_NotificationMutex; + static inline std::condition_variable m_NotificationCv; + static inline std::size_t m_NumNotifications{}; + static inline NotificationType m_LastNotification{}; + + Host::Ptr m_Host; + Notification::Ptr m_Notification; + TimePeriod::Ptr m_TimePeriod; + Dictionary::Ptr m_AllTimePeriod; +}; + +BOOST_FIXTURE_TEST_SUITE(notificationcomponent, NotificationComponentFixture); + +/* Test sending out reminder notifications in a given interval. + */ +BOOST_AUTO_TEST_CASE(notify_send_reminders) +{ + SetNotificationInverval(0.15); + ReceiveCheckResults(2, ServiceCritical); + + // The first run of the timer sets up the next reminder notification. + NotificationTimerHandler(); + BOOST_REQUIRE(WaitForExpectedNotificationCount(1)); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + + // Rerunning the timer before the next interval should not trigger a reminder notification. + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + + // After waiting until the interval has passed, a reminder will be queued. + WaitUntilNextReminderScheduled(); + NotificationTimerHandler(); + BOOST_REQUIRE(WaitForExpectedNotificationCount(2)); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + + // Now we test that reminders are only sent for Critical states. + // Hard state is switched to OK. + ReceiveCheckResults(1, ServiceOK); + NotificationTimerHandler(); + BOOST_REQUIRE(WaitForExpectedNotificationCount(3)); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationRecovery); + + // Now we wait for one interval and check that no reminder has been sent. + WaitUntilNextReminderScheduled(); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 3); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationRecovery); +} + +/* Tests simple sending of notifications on each state change. + */ +BOOST_AUTO_TEST_CASE(notify_simple) +{ + BeginTimePeriod(); + + ReceiveCheckResults(2, ServiceCritical); + NotificationTimerHandler(); + BOOST_REQUIRE(WaitForExpectedNotificationCount(1)); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + + ReceiveCheckResults(1, ServiceCritical); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + + ReceiveCheckResults(1, ServiceOK); + NotificationTimerHandler(); + BOOST_REQUIRE(WaitForExpectedNotificationCount(2)); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationRecovery); + + ReceiveCheckResults(2, ServiceOK); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 2); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationRecovery); +} + +/* This tests the simplest case where a suppressed notification will be sent after resuming + * a TimePeriod. A single event occurs outside the TimePeriod and the notification should be + * sent as soon as the timer runs after the TimePeriod is resumed. + */ +BOOST_AUTO_TEST_CASE(notify_after_timeperiod_simple) +{ + BeginTimePeriod(); + + ReceiveCheckResults(1, ServiceOK); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 0); + BOOST_REQUIRE_EQUAL(GetLastNotification(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + + EndTimePeriod(); + + ReceiveCheckResults(3, ServiceCritical); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 0); + BOOST_REQUIRE_EQUAL(GetLastNotification(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationProblem); + + BeginTimePeriod(); + NotificationTimerHandler(); + BOOST_REQUIRE(WaitForExpectedNotificationCount(1)); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); +} + +/* Similar to the test-case above, but has multiple state-changes outside of the TimePeriod + * This is important, since there are multiple places in the code that check on and make modifications + * to the list of suppressed events. A concrete example of a bug like this is #10575. + */ +BOOST_AUTO_TEST_CASE(notify_multiple_state_changes_outside_timeperiod) +{ + BOOST_REQUIRE_EQUAL(GetLastNotificationTimestamp(), 0.0); + + BeginTimePeriod(); + ReceiveCheckResults(2, ServiceCritical); + + BOOST_REQUIRE(WaitForExpectedNotificationCount(1)); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + + EndTimePeriod(); + + ReceiveCheckResults(1, ServiceOK); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationRecovery); + + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationRecovery); + + ReceiveCheckResults(1, ServiceCritical); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationRecovery); + + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + // NOTE: This is the bug. The timer run above has reset the suppressed ServiceOK event + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + + // Third Critical check result will set the Critical hard state. + ReceiveCheckResults(2, ServiceCritical); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationProblem); + + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationProblem); + + ReceiveCheckResults(1, ServiceOK); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + + // Resonably we would now expect a NotificationRecovered, but it will not arrive due to the bug. + BeginTimePeriod(); + + // No notification will be received because the suppressed notifications got borked + // when they were cleared previously in FireSuppressedNotifications() + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); +} + +/* This tests if suppressed notifications of opposite types cancel each other out. + */ +BOOST_AUTO_TEST_CASE(no_notify_suppressed_cancel_out) +{ + BOOST_REQUIRE_EQUAL(GetLastNotificationTimestamp(), 0.0); + + BeginTimePeriod(); + + ReceiveCheckResults(1, ServiceOK); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 0); + BOOST_REQUIRE_EQUAL(GetLastNotification(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + + EndTimePeriod(); + + ReceiveCheckResults(3, ServiceCritical); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 0); + BOOST_REQUIRE_EQUAL(GetLastNotification(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationProblem); + + ReceiveCheckResults(1, ServiceOK); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 0); + BOOST_REQUIRE_EQUAL(GetLastNotification(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + + BeginTimePeriod(); + + // Ensure no notification is sent after resuming the TimePeriod. + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 0); + BOOST_REQUIRE_EQUAL(GetLastNotification(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + + // Now repeat the same starting from a Critical state + ReceiveCheckResults(3, ServiceCritical); + NotificationTimerHandler(); + BOOST_REQUIRE(WaitForExpectedNotificationCount(1)); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + + EndTimePeriod(); + + ReceiveCheckResults(1, ServiceOK); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationRecovery); + + ReceiveCheckResults(3, ServiceCritical); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + + BeginTimePeriod(); + + // Ensure no notification is sent after resuming the TimePeriod. + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); +} + +/* This may look similar to the test-case above, but the critical difference is that here + * the final state change happens inside the TimePeriod again, but before the timer runs. + * The outdated suppressed NotificationProblem will then be subtracted when the timer runs. + */ +BOOST_AUTO_TEST_CASE(no_notify_non_applicable_reason) +{ + BOOST_REQUIRE_EQUAL(GetLastNotificationTimestamp(), 0.0); + + BeginTimePeriod(); + + ReceiveCheckResults(1, ServiceOK); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 0); + BOOST_REQUIRE_EQUAL(GetLastNotification(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + + EndTimePeriod(); + + // We queue a suppressed notification. + ReceiveCheckResults(3, ServiceCritical); + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 0); + BOOST_REQUIRE_EQUAL(GetLastNotification(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationProblem); + + BeginTimePeriod(); + + // In this scenario a check result that goes against the suppressed notification is processed + // before the timer can run again. No notification should be sent, because the last state + // change the user was notified about was the same. + ReceiveCheckResults(1, ServiceOK); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 0); + BOOST_REQUIRE_EQUAL(GetLastNotification(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationProblem); + + // When the timer runs, it should clear the suppressed notification but not send anything. + NotificationTimerHandler(); + BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); + BOOST_REQUIRE_EQUAL(GetNotificationCount(), 0); + BOOST_REQUIRE_EQUAL(GetLastNotification(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); +} + +BOOST_AUTO_TEST_SUITE_END() From a9c139f5c5ba6bf81bc7f240840d524bb01c0332 Mon Sep 17 00:00:00 2001 From: Johannes Schmidt Date: Tue, 21 Oct 2025 12:31:01 +0200 Subject: [PATCH 3/4] Subtract inapplicable suppressed notifications at a later point Without this commit, every time the NotificationTimerHandler runs it will discard notifications that don't apply to the reason of the latest check result. This is probably intended to clear outdated suppressed notifications immediately when the TimePeriod resumes, but it also clears out important ones (see the test case). This commit fixes that by clearing out inapplicable notifications when the timer runs the first time after the TimePeriod resumes. By that time we can expect that no new suppressed notifications will be added and all notifications that don't conflict with the last check-result can still be run. Fixes #10575 --- lib/notification/notificationcomponent.cpp | 49 ++++++++++------------ 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/lib/notification/notificationcomponent.cpp b/lib/notification/notificationcomponent.cpp index 982a8385fa..58c3fc28a0 100644 --- a/lib/notification/notificationcomponent.cpp +++ b/lib/notification/notificationcomponent.cpp @@ -83,37 +83,34 @@ void FireSuppressedNotifications(const Notification::Ptr& notification) int subtract = 0; auto checkable (notification->GetCheckable()); - for (auto type : {NotificationProblem, NotificationRecovery, NotificationFlappingStart, NotificationFlappingEnd}) { - if ((suppressedTypes & type) && !checkable->NotificationReasonApplies(type)) { - subtract |= type; - suppressedTypes &= ~type; - } - } + auto tp (notification->GetPeriod()); - if (suppressedTypes) { - auto tp (notification->GetPeriod()); + if ((!tp || tp->IsInside(Utility::GetTime())) && !checkable->IsLikelyToBeCheckedSoon()) { + for (auto type : {NotificationProblem, NotificationRecovery, NotificationFlappingStart, NotificationFlappingEnd}) { + if (!(suppressedTypes & type) || checkable->NotificationReasonSuppressed(type)) { + continue; + } - if ((!tp || tp->IsInside(Utility::GetTime())) && !checkable->IsLikelyToBeCheckedSoon()) { - for (auto type : {NotificationProblem, NotificationRecovery, NotificationFlappingStart, NotificationFlappingEnd}) { - if (!(suppressedTypes & type) || checkable->NotificationReasonSuppressed(type)) - continue; + if (!checkable->NotificationReasonApplies(type)) { + subtract |= type; + continue; + } - auto notificationName (notification->GetName()); + auto notificationName (notification->GetName()); - Log(LogNotice, "NotificationComponent") - << "Attempting to re-send previously suppressed notification '" << notificationName << "'."; + Log(LogNotice, "NotificationComponent") + << "Attempting to re-send previously suppressed notification '" << notificationName << "'."; - subtract |= type; - SubtractSuppressedNotificationTypes(notification, subtract); - subtract = 0; - - try { - notification->BeginExecuteNotification(type, checkable->GetLastCheckResult(), false, false); - } catch (const std::exception& ex) { - Log(LogWarning, "NotificationComponent") - << "Exception occurred during notification for object '" - << notificationName << "': " << DiagnosticInformation(ex, false); - } + subtract |= type; + SubtractSuppressedNotificationTypes(notification, subtract); + subtract = 0; + + try { + notification->BeginExecuteNotification(type, checkable->GetLastCheckResult(), false, false); + } catch (const std::exception& ex) { + Log(LogWarning, "NotificationComponent") + << "Exception occurred during notification for object '" + << notificationName << "': " << DiagnosticInformation(ex, false); } } } From 75c7d28bb1f4674d8bcbbc79c4b3ab3c480019b3 Mon Sep 17 00:00:00 2001 From: Johannes Schmidt Date: Tue, 21 Oct 2025 12:34:34 +0200 Subject: [PATCH 4/4] Adapt the unit-test to reflect fix in the previous commit --- test/notification-notificationcomponent.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/test/notification-notificationcomponent.cpp b/test/notification-notificationcomponent.cpp index 880ef92525..c9243581c8 100644 --- a/test/notification-notificationcomponent.cpp +++ b/test/notification-notificationcomponent.cpp @@ -350,37 +350,32 @@ BOOST_AUTO_TEST_CASE(notify_multiple_state_changes_outside_timeperiod) BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); - // NOTE: This is the bug. The timer run above has reset the suppressed ServiceOK event - BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationRecovery); // Third Critical check result will set the Critical hard state. ReceiveCheckResults(2, ServiceCritical); BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); - BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); NotificationTimerHandler(); BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); - BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationProblem); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); ReceiveCheckResults(1, ServiceOK); BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); - BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); + BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), NotificationRecovery); - // Resonably we would now expect a NotificationRecovered, but it will not arrive due to the bug. BeginTimePeriod(); - // No notification will be received because the suppressed notifications got borked - // when they were cleared previously in FireSuppressedNotifications() NotificationTimerHandler(); - BOOST_REQUIRE(AssertNoAttemptedSendLogPattern()); - BOOST_REQUIRE_EQUAL(GetNotificationCount(), 1); - BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationProblem); + BOOST_REQUIRE(WaitForExpectedNotificationCount(2)); + BOOST_REQUIRE_EQUAL(GetLastNotification(), NotificationRecovery); BOOST_REQUIRE_EQUAL(GetSuppressedNotifications(), 0); }