Skip to content

refactor(thold): extract the notification delivery from the threshold check #801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
somethingwithproof merged 4 commits into from
Aug 18, 2026
Merged

refactor(thold): extract the notification delivery from the threshold check #801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c4a8f1
refactor(thold): extract the notification delivery from the threshold…
somethingwithproof Aug 17, 2026
4689783
test: pin the alert-to-warning downgrade
somethingwithproof Aug 17, 2026
8923a49
docs: remove orphaned evaluation context block
somethingwithproof Aug 17, 2026
c468347
ci: retry transient dependency downloads
somethingwithproof Aug 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions .github/workflows/php-unit-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: |
Expand Down
13 changes: 12 additions & 1 deletion .github/workflows/plugin-ci-workflow.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
193 changes: 193 additions & 0 deletions tests/Unit/TholdMailNotificationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
<?php
/*
+-------------------------------------------------------------------------+
| Copyright (C) 2004-2026 The Cacti Group |
| |
| This program is free software; you can redistribute it and/or |
| modify it under the terms of the GNU General Public License |
| as published by the Free Software Foundation; either version 2 |
| of the License, or (at your option) any later version. |
+-------------------------------------------------------------------------+
| Cacti: The Complete RRDtool-based Graphing Solution |
+-------------------------------------------------------------------------+
| http://www.cacti.net/ |
+-------------------------------------------------------------------------+
*/

/**
* Delivery of one notification to one recipient list.
*
* This was the block repeated at every send site in thold_check_threshold(),
* so the rules it carries -- when a mail is skipped, which body is built --
* could only be reached by driving a whole poll.
*/
final class TholdMailNotificationTest extends TestCase {
/**
* @return void
*/
public static function setUpBeforeClass(): void {
self::loadPluginSource('thold_functions.php');
}

/**
* @param array<string, mixed> $overrides
*
* @return array<string, mixed>
*/
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<string, mixed>
*/
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<string, mixed> $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<string, array{0: string}>
*/
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<string, array{0: string, 1: string}>
*/
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);
}
}
18 changes: 18 additions & 0 deletions tests/Unit/ThresholdHiLowCharacterizationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down
92 changes: 47 additions & 45 deletions thold_functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2202,16 +2202,53 @@ function thold_datasource_required($name, $data_source) {
}

/**
* Gather the settings a threshold evaluation reads, in one place.
* Deliver one notification to one recipient list.
*
* 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.
* 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<string, mixed> $file_array Graph attachment, or empty for none.
* @param array<string, mixed> $thold_data Threshold row.
* @param array<string, mixed> $h Device row.
* @param int $timespan Graph timespan for the attachment.
*
* @return array<string, mixed>
* @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');
Expand Down Expand Up @@ -2565,14 +2602,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',
Expand Down Expand Up @@ -2627,14 +2657,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'];
Expand Down Expand Up @@ -2885,14 +2908,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'];
Expand Down Expand Up @@ -3003,14 +3019,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'];
Expand Down Expand Up @@ -3232,14 +3241,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'];
Expand Down
Loading