From 0c4a8f1d9e90c7b5e0b692ea4cb165d587593c73 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 21:29:00 -0700 Subject: [PATCH 1/4] refactor(thold): extract the notification delivery from the threshold check Every send site resolved the notification list's format file, then mailed the list unless it was empty or the threshold had been acknowledged. That pair is now thold_mail_notification(), and the five sites whose shape matched exactly call it. The body is still built inside the guard rather than by the caller, because composing it resolves the device's site and an empty recipient list should not pay for a message nobody receives. The function returns the message so the sites that mail a second list can reuse it as before. One observable difference: where the primary list is empty, $message is now '' rather than unset. The second mail already read that unset variable, so this removes an undefined-variable read without changing what the mail contains. thold_check_threshold() goes from 1378 lines to 1343. Signed-off-by: Thomas Vincent --- tests/Unit/TholdMailNotificationTest.php | 193 +++++++++++++++++++++++ thold_functions.php | 101 +++++++----- 2 files changed, 254 insertions(+), 40 deletions(-) create mode 100644 tests/Unit/TholdMailNotificationTest.php diff --git a/tests/Unit/TholdMailNotificationTest.php b/tests/Unit/TholdMailNotificationTest.php new file mode 100644 index 00000000..8ca4eb5c --- /dev/null +++ b/tests/Unit/TholdMailNotificationTest.php @@ -0,0 +1,193 @@ + $overrides + * + * @return array + */ + private function threshold(array $overrides = []) { + return $overrides + [ + 'id' => 1, + 'name_cache' => 'CPU', + 'data_source_name' => 'traffic_in', + 'lastread' => 95, + 'local_graph_id' => 7, + 'acknowledgment' => '', + 'notes' => '', + 'dnotes' => '', + 'external_id' => '', + 'thold_type' => 0, + 'thold_hi' => 90, + 'thold_low' => 10, + 'thold_fail_trigger' => 3, + 'email_body' => '', + ]; + } + + /** + * @return array + */ + private function device() { + return [ + 'id' => 2, + 'description' => 'core-switch-1', + 'hostname' => '10.0.0.1', + 'location' => 'rack 4', + 'site_id' => 1, + ]; + } + + /** + * @param string $recipients + * @param array $overrides + * @param string $type + * + * @return string + */ + private function deliver($recipients, array $overrides = [], $type = 'alert') { + $thold = $this->threshold($overrides); + $device = $this->device(); + + return thold_mail_notification($recipients, 'bcc@example.org', 'ALERT: CPU', $type, 4, [], $thold, $device); + } + + /** + * @return void + */ + public function testRecipientsReceiveTheNotification(): void { + $this->deliver('ops@example.org'); + + $this->assertCount(1, CactiStubs::$mail); + $this->assertSame('ops@example.org', CactiStubs::$mail[0]['to']); + $this->assertSame('bcc@example.org', CactiStubs::$mail[0]['bcc']); + $this->assertSame('ALERT: CPU', CactiStubs::$mail[0]['subject']); + } + + /** + * @return array + */ + public static function emptyRecipientProvider() { + return [ + 'empty string' => [''], + 'whitespace' => [' '], + ]; + } + + /** + * @dataProvider emptyRecipientProvider + * + * @param string $recipients + * + * @return void + */ + public function testNothingIsSentWithoutRecipients($recipients): void { + $this->assertSame('', $this->deliver($recipients)); + $this->assertSame([], CactiStubs::$mail); + } + + /** + * An acknowledged threshold has already been seen by an operator, so it + * stops mailing even while it keeps breaching. + * + * @return void + */ + public function testAcknowledgedThresholdIsNotMailed(): void { + $this->assertSame('', $this->deliver('ops@example.org', ['acknowledgment' => 'on'])); + $this->assertSame([], CactiStubs::$mail); + } + + /** + * Building the body queries, so an empty recipient list must not pay for a + * message nobody receives. + * + * @return void + */ + public function testNoMessageIsBuiltWhenThereIsNoOneToMail(): void { + $this->deliver(''); + + // Composing a body resolves the device's site; skipping it must not. + $site_lookups = array_filter(CactiStubs::$calls, static function ($call) { + return strpos($call['sql'], 'FROM sites') !== false; + }); + + $this->assertSame([], array_values($site_lookups)); + } + + /** + * @return array + */ + public static function textTypeProvider() { + return [ + 'alert' => ['alert', 'thold_alert_text'], + 'warning' => ['warning', 'thold_warning_text'], + 'restoral' => ['restoral', 'thold_restoral_text'], + ]; + } + + /** + * Each class of notification takes its body from its own setting. + * + * @dataProvider textTypeProvider + * + * @param string $type + * @param string $option + * + * @return void + */ + public function testEachNotificationClassUsesItsOwnBody($type, $option): void { + CactiStubs::$configOptions[$option] = 'body for ' . $type; + + $this->assertSame('body for ' . $type, $this->deliver('ops@example.org', [], $type)); + } + + /** + * @return void + */ + public function testTheSentMessageIsReturnedForReuse(): void { + CactiStubs::$configOptions['thold_alert_text'] = 'the body'; + + $this->assertSame('the body', $this->deliver('ops@example.org')); + } + + /** + * @return void + */ + public function testTheListFormatIsResolvedEvenWhenNothingIsSent(): void { + $this->deliver(''); + + $format_lookups = array_filter(CactiStubs::$calls, static function ($call) { + return strpos($call['sql'], 'format_file') !== false; + }); + + $this->assertNotEmpty($format_lookups); + } +} diff --git a/thold_functions.php b/thold_functions.php index f195d302..542739df 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -2209,9 +2209,65 @@ function thold_datasource_required($name, $data_source) { * out of thold_check_threshold() without changing behaviour. * * @param array $thold_data Threshold row. + * @param mixed $recipients + * @param mixed $bcc + * @param mixed $subject + * @param mixed $text_type + * @param mixed $list_id + * @param mixed $file_array + * @param mixed $h + * @param mixed $timespan * * @return array */ +/** + * Deliver one notification to one recipient list. + * + * The eighteen send sites in thold_check_threshold() all resolve the list's + * format file, then mail the list unless it is empty or the threshold has been + * acknowledged. The message body is built inside that guard rather than by the + * caller, because building it queries and an empty recipient list should not + * pay for a message nobody receives. + * + * @param string $recipients Comma separated addresses, possibly empty. + * @param string $bcc Comma separated blind addresses. + * @param string $subject Subject line, already composed. + * @param string $text_type alert, warning or restoral. + * @param int $list_id Notification list supplying the format. + * @param array $file_array Graph attachment, or empty for none. + * @param array $thold_data Threshold row. + * @param array $h Device row. + * @param int $timespan Graph timespan for the attachment. + * + * @return string The message that was sent, or '' when nothing was. + */ +function thold_mail_notification($recipients, $bcc, $subject, $text_type, $list_id, $file_array, &$thold_data, &$h, $timespan = 7) { + $format_file = thold_get_thold_notification_format_file($thold_data['id'], $list_id); + + if (trim($recipients) == '' || $thold_data['acknowledgment'] != '') { + return ''; + } + + switch ($text_type) { + case 'alert': + $message = get_thold_alert_text($thold_data['data_source_name'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id']); + + break; + case 'warning': + $message = get_thold_warning_text($thold_data['data_source_name'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id']); + + break; + default: + $message = get_thold_restoral_text($thold_data['data_source_name'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id']); + + break; + } + + thold_mail($recipients, $bcc, '', $subject, $message, $file_array, '', $list_id, $h, $format_file, $timespan); + + return $message; +} + function thold_evaluation_context(array $thold_data) { $alert_trigger = read_config_option('alert_trigger'); $httpurl = read_config_option('base_url'); @@ -2565,14 +2621,7 @@ function thold_check_threshold(&$thold_data) { logger($subject, $url, $syslog_priority, $syslog_facility); } - $notify_list_id = $thold_data['notify_warning']; - $format_file = thold_get_thold_notification_format_file($thold_data['id'], $notify_list_id); - - if (trim($warning_emails) != '' && $thold_data['acknowledgment'] == '') { - $message = get_thold_warning_text($thold_data['data_source_name'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id']); - - thold_mail($warning_emails, $warning_bcc_emails, '', $subject, $message, $file_array, '', $notify_list_id, $h, $format_file, $thold_data['graph_timespan']); - } + $message = thold_mail_notification($warning_emails, $warning_bcc_emails, $subject, 'warning', $thold_data['notify_warning'], $file_array, $thold_data, $h, $thold_data['graph_timespan']); $save = [ 'class' => 'warn', @@ -2627,14 +2676,7 @@ function thold_check_threshold(&$thold_data) { $subject = get_email_subject('ALERT > WARNING', false, $lastread, $ra, $warning_breach_up, $thold_data); if (!$suspend_notify && !$maint_dev) { - $notify_list_id = $thold_data['notify_alert']; - $format_file = thold_get_thold_notification_format_file($thold_data['id'], $notify_list_id); - - if (trim($alert_emails) != '' && $thold_data['acknowledgment'] == '') { - $message = get_thold_warning_text($thold_data['data_source_name'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id']); - - thold_mail($alert_emails, $alert_bcc_emails, '', $subject, $message, $file_array, '', $notify_list_id, $h, $format_file, $thold_data['graph_timespan']); - } + $message = thold_mail_notification($alert_emails, $alert_bcc_emails, $subject, 'warning', $thold_data['notify_alert'], $file_array, $thold_data, $h, $thold_data['graph_timespan']); if ($notify_different) { $notify_list_id = $thold_data['notify_warning']; @@ -2885,14 +2927,7 @@ function thold_check_threshold(&$thold_data) { logger($subject, $url, $syslog_priority, $syslog_facility); } - $notify_list_id = $thold_data['notify_alert']; - $format_file = thold_get_thold_notification_format_file($thold_data['id'], $notify_list_id); - - if (trim($alert_emails) != '' && $thold_data['acknowledgment'] == '') { - $message = get_thold_restoral_text($thold_data['data_source_name'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id']); - - thold_mail($alert_emails, $alert_bcc_emails, '', $subject, $message, $file_array, '', $notify_list_id, $h, $format_file, $thold_data['graph_timespan']); - } + $message = thold_mail_notification($alert_emails, $alert_bcc_emails, $subject, 'restoral', $thold_data['notify_alert'], $file_array, $thold_data, $h, $thold_data['graph_timespan']); if ($notify_different) { $notify_list_id = $thold_data['notify_warning']; @@ -3003,14 +3038,7 @@ function thold_check_threshold(&$thold_data) { logger($subject, $url, $syslog_priority, $syslog_facility); } - $notify_list_id = $thold_data['notify_alert']; - $format_file = thold_get_thold_notification_format_file($thold_data['id'], $notify_list_id); - - if (trim($alert_emails) != '' && $thold_data['acknowledgment'] == '') { - $message = get_thold_alert_text($thold_data['data_source_name'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id']); - - thold_mail($alert_emails, $alert_bcc_emails, '', $subject, $message, $file_array, '', $notify_list_id, $h, $format_file, $thold_data['graph_timespan']); - } + $message = thold_mail_notification($alert_emails, $alert_bcc_emails, $subject, 'alert', $thold_data['notify_alert'], $file_array, $thold_data, $h, $thold_data['graph_timespan']); if ($notify_different) { $notify_list_id = $thold_data['notify_warning']; @@ -3232,14 +3260,7 @@ function thold_check_threshold(&$thold_data) { logger($subject, $url, $syslog_priority, $syslog_facility); } - $notify_list_id = $thold_data['notify_alert']; - $format_file = thold_get_thold_notification_format_file($thold_data['id'], $notify_list_id); - - if (trim($alert_emails) != '' && $thold_data['acknowledgment'] == '') { - $message = get_thold_alert_text($thold_data['data_source_name'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id']); - - thold_mail($alert_emails, $alert_bcc_emails, '', $subject, $message, $file_array, '', $notify_list_id, $h, $format_file, $thold_data['graph_timespan']); - } + $message = thold_mail_notification($alert_emails, $alert_bcc_emails, $subject, 'alert', $thold_data['notify_alert'], $file_array, $thold_data, $h, $thold_data['graph_timespan']); if ($notify_different) { $notify_list_id = $thold_data['notify_warning']; From 4689783bcecf9a1f0822c38c949de9f90cbfe75a Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 21:30:07 -0700 Subject: [PATCH 2/4] test: pin the alert-to-warning downgrade A threshold already in alert whose reading falls back into the warning band notifies a downgrade rather than a restoral. Reaching it needs both fail counts at or above their triggers on the same poll, which none of the earlier scenarios set up. Signed-off-by: Thomas Vincent --- .../ThresholdHiLowCharacterizationTest.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/Unit/ThresholdHiLowCharacterizationTest.php b/tests/Unit/ThresholdHiLowCharacterizationTest.php index 85eb1004..d23613f5 100644 --- a/tests/Unit/ThresholdHiLowCharacterizationTest.php +++ b/tests/Unit/ThresholdHiLowCharacterizationTest.php @@ -136,6 +136,24 @@ public function testReadingBetweenWarningAndAlertBoundsNotifiesTheWarning(): voi $this->assertSame([ST_NOTIFYWA], $outcome->logStatuses()); } + /** + * A threshold already in alert whose reading falls back into the warning + * band is a de-escalation, not a restoral, and gets its own notification. + * + * @return void + */ + public function testFallingFromAlertIntoTheWarningBandNotifiesTheDowngrade(): void { + $outcome = $this->bounded([ + 'lastread' => 85, + 'thold_alert' => STAT_HI, + 'thold_fail_count' => 5, + 'thold_warning_fail_count' => 5, + ])->poll(); + + $this->assertSame([ST_NOTIFYAW], $outcome->logStatuses()); + $this->assertStringStartsWith('ALERT > WARNING', $outcome->subjects()[0]); + } + /** * @return void */ From 8923a4929426db146312896e36ec1772f5375229 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:50:47 -0700 Subject: [PATCH 3/4] docs: remove orphaned evaluation context block --- thold_functions.php | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/thold_functions.php b/thold_functions.php index 542739df..019da05a 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -2201,25 +2201,6 @@ function thold_datasource_required($name, $data_source) { return true; } -/** - * Gather the settings a threshold evaluation reads, in one place. - * - * Everything here is derived from the threshold row and the Cacti settings; it - * does not decide anything and has no side effects, which is what lets it move - * out of thold_check_threshold() without changing behaviour. - * - * @param array $thold_data Threshold row. - * @param mixed $recipients - * @param mixed $bcc - * @param mixed $subject - * @param mixed $text_type - * @param mixed $list_id - * @param mixed $file_array - * @param mixed $h - * @param mixed $timespan - * - * @return array - */ /** * Deliver one notification to one recipient list. * From c468347ba27fa7e94ac36a04fc7d9af29e6c0c7b Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 18:28:34 -0700 Subject: [PATCH 4/4] ci: retry transient dependency downloads --- .github/workflows/php-unit-tests.yml | 15 +++++++++++++-- .github/workflows/plugin-ci-workflow.yml | 13 ++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/.github/workflows/php-unit-tests.yml b/.github/workflows/php-unit-tests.yml index 89eb1ad4..da6cba4a 100644 --- a/.github/workflows/php-unit-tests.yml +++ b/.github/workflows/php-unit-tests.yml @@ -72,8 +72,19 @@ jobs: # Thold contributes no parallel vendor directory or PHPUnit dependency. - name: Build Cacti test image run: | - docker build --tag cacti-web --file cacti-toolchain/docker/Dockerfile cacti-toolchain/docker - docker build --tag cacti-thold-test --file cacti-toolchain/docker/Dockerfile.test cacti-toolchain + for attempt in 1 2 3; do + if docker build --tag cacti-web --file cacti-toolchain/docker/Dockerfile cacti-toolchain/docker && \ + docker build --tag cacti-thold-test --file cacti-toolchain/docker/Dockerfile.test cacti-toolchain; then + exit 0 + fi + + if [ "$attempt" -lt 3 ]; then + sleep 10 + fi + done + + echo 'Cacti test image build failed after three attempts.' >&2 + exit 1 - name: Lint every PHP source file run: | diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index e17554e8..73630b54 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -149,7 +149,18 @@ jobs: run: | cd ${{ github.workspace }}/cacti if [ -f composer.json ]; then - sudo composer install --prefer-dist --no-progress + for attempt in 1 2 3; do + if sudo composer install --prefer-dist --no-progress --no-interaction; then + exit 0 + fi + + if [ "$attempt" -lt 3 ]; then + sleep 10 + fi + done + + echo 'Composer install failed after three attempts.' >&2 + exit 1 fi - name: Create Cacti config.php