From dceb5ab7f6bedcfa6cb9a5e5b02b7165cfdcb516 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 19:09:53 -0700 Subject: [PATCH 1/7] Retry failed queued email notifications --- CHANGELOG.md | 1 + INFO | 2 +- README.md | 6 ++ includes/database.php | 24 +++++ notify_queue.php | 14 +++ tests/Unit/NotificationQueueClaimTest.php | 2 +- tests/Unit/NotificationQueueRetryTest.php | 125 ++++++++++++++++++++++ tests/bootstrap-unit.php | 2 +- thold_functions.php | 84 +++++++++------ thold_notify.php | 1 + 10 files changed, 228 insertions(+), 33 deletions(-) create mode 100644 tests/Unit/NotificationQueueRetryTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b7db7d7..03f96bc7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ --- develop --- +* issue#784: Retry failed queued email notifications with bounded exponential backoff * issue#686: Applying a templated threshold to a graph via the wrench icon, creates a duplicate graph * issue#707: Excessive timeout for row caching prevents data from being updated timely * issue#710: Fixing Typo in thold_daemons.service File diff --git a/INFO b/INFO index b6c2c483..7099aeb2 100644 --- a/INFO +++ b/INFO @@ -21,7 +21,7 @@ [info] name = thold -version = 1.8.2 +version = 1.8.3 longname = Thresholds author = The Cacti Group email = diff --git a/README.md b/README.md index 46310dce..a5e95d7c 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,12 @@ and become familiar with its settings. From there, you can provide overall control of thold, and set defaults for things like Email bodies, weekend exemptions, alert log retention, logging, etc. +When the notification queue is enabled, transient email failures are retried +up to five times with exponential backoff from one minute to one hour. The +Notification Queue page shows the attempt count and next eligible retry time. +After the fifth failed attempt the row becomes a terminal error so a permanent +SMTP or address failure cannot retry forever. + As with much of Cacti, settings should be documented in line with the actual setting. If you find that any of these settings are ambiguous, please create a pull request with your proposed changes. diff --git a/includes/database.php b/includes/database.php index 279aff65..6512e6f4 100644 --- a/includes/database.php +++ b/includes/database.php @@ -1755,6 +1755,27 @@ function thold_upgrade_database($force = false) { db_execute('UPDATE plugin_notification_lists SET enabled = "on"'); } + if (cacti_version_compare($oldv, '1.8.3', '<')) { + db_add_column('notification_queue', [ + 'name' => 'attempt_count', + 'type' => 'int', + 'unsigned' => true, + 'NULL' => false, + 'default' => '0', + 'after' => 'error_message'] + ); + + db_add_column('notification_queue', [ + 'name' => 'next_attempt', + 'type' => 'timestamp', + 'NULL' => true, + 'default' => null, + 'after' => 'attempt_count'] + ); + + db_add_index('notification_queue', 'INDEX', 'retry_ready', ['event_processed', 'process_id', 'next_attempt']); + } + db_add_column('thold_data', [ 'name' => 'external_id', 'type' => 'varchar(20)', @@ -2123,12 +2144,15 @@ function thold_setup_database() { $data['columns'][] = ['name' => 'event_data', 'type' => 'longblob', 'NULL' => false, 'default' => '']; $data['columns'][] = ['name' => 'error_code', 'type' => 'int', 'NULL' => false, 'default' => '0']; $data['columns'][] = ['name' => 'error_message', 'type' => 'varchar(128)', 'NULL' => false, 'default' => '']; + $data['columns'][] = ['name' => 'attempt_count', 'type' => 'int', 'unsigned' => true, 'NULL' => false, 'default' => '0']; + $data['columns'][] = ['name' => 'next_attempt', 'type' => 'timestamp', 'NULL' => true, 'default' => null]; $data['columns'][] = ['name' => 'process_id', 'type' => 'int', 'unsigned' => true, 'NULL' => false, 'default' => '0']; $data['columns'][] = ['name' => 'event_processed', 'type' => 'tinyint', 'unsigned' => true, 'NULL' => false, 'default' => '0']; $data['columns'][] = ['name' => 'event_processed_time', 'type' => 'timestamp', 'NULL' => false, 'default' => '0000-00-00']; $data['columns'][] = ['name' => 'event_processed_runtime', 'type' => 'double', 'unsigned' => true, 'NULL' => false, 'default' => '0']; $data['primary'] = 'id'; $data['keys'][] = ['name' => 'topic_processed', 'columns' => 'topic`, `event_processed']; + $data['keys'][] = ['name' => 'retry_ready', 'columns' => 'event_processed`, `process_id`, `next_attempt']; $data['keys'][] = ['name' => 'process_id', 'columns' => 'process_id']; $data['keys'][] = ['name' => 'object_id', 'columns' => 'object_id']; $data['keys'][] = ['name' => 'host_id', 'columns' => 'host_id']; diff --git a/notify_queue.php b/notify_queue.php index 12f24645..73025bb2 100644 --- a/notify_queue.php +++ b/notify_queue.php @@ -463,6 +463,18 @@ function clearFilter() { 'sort' => 'DESC', 'tip' => __('Did this notification result in an error. Hover on the error column for details.', 'thold') ], + 'attempt_count' => [ + 'display' => __('Attempts', 'thold'), + 'align' => 'right', + 'sort' => 'DESC', + 'tip' => __('The number of delivery attempts made for this notification.', 'thold') + ], + 'next_attempt' => [ + 'display' => __('Next Attempt', 'thold'), + 'align' => 'right', + 'sort' => 'DESC', + 'tip' => __('When a failed notification is eligible for its next retry.', 'thold') + ], 'event_processed_runtime' => [ 'display' => __('Run Time', 'thold'), 'align' => 'right', @@ -492,6 +504,8 @@ function clearFilter() { form_selectable_cell($n['id'], $n['id'], '', 'right'); form_selectable_cell($n['event_time'], $n['id'], '', 'right'); form_selectable_cell($n['event_processed'] == 0 ? __('Pending', 'thold') : __('Done', 'thold'), $n['id'], '', 'right'); + form_selectable_cell((int) ($n['attempt_count'] ?? 0), $n['id'], '', 'right'); + form_selectable_cell($n['next_attempt'] ?: __('N/A', 'thold'), $n['id'], '', 'right'); if ($n['event_processed'] > 0) { form_selectable_cell($n['error_code'] > 0 ? __('Errored', 'thold') : __('Success', 'thold'), $n['id'], '', 'right'); diff --git a/tests/Unit/NotificationQueueClaimTest.php b/tests/Unit/NotificationQueueClaimTest.php index fe1e969a..7a1e83b7 100644 --- a/tests/Unit/NotificationQueueClaimTest.php +++ b/tests/Unit/NotificationQueueClaimTest.php @@ -118,7 +118,7 @@ public function testTheClaimTakesOnlyUnheldRows(): void { $src = file_get_contents(dirname(__DIR__, 2) . '/thold_notify.php'); $this->assertMatchesRegularExpression( - '/SET process_id = \?\s+WHERE event_processed = 0\s+AND process_id = 0/', + '/SET process_id = \?\s+WHERE event_processed = 0\s+AND \(next_attempt IS NULL OR next_attempt <= NOW\(\)\)\s+AND process_id = 0/', $src ); } diff --git a/tests/Unit/NotificationQueueRetryTest.php b/tests/Unit/NotificationQueueRetryTest.php new file mode 100644 index 00000000..dee3fe08 --- /dev/null +++ b/tests/Unit/NotificationQueueRetryTest.php @@ -0,0 +1,125 @@ + + */ + private function lastPreparedCall() { + $calls = array_values(array_filter(CactiStubs::$calls, static function ($call) { + return $call['fn'] === 'db_execute_prepared'; + })); + + $this->assertNotEmpty($calls); + + return end($calls); + } + + /** + * @return void + */ + public function testRetryDelayUsesBoundedExponentialBackoff(): void { + $this->assertSame(60, thold_notification_retry_delay(1)); + $this->assertSame(120, thold_notification_retry_delay(2)); + $this->assertSame(480, thold_notification_retry_delay(4)); + $this->assertSame(3600, thold_notification_retry_delay(7)); + } + + /** + * @return void + */ + public function testSuccessfulDeliveryIsTerminal(): void { + thold_notification_record_delivery(42, '', 0.25, 2); + + $call = $this->lastPreparedCall(); + $sql = preg_replace('/\s+/', ' ', $call['sql']); + + $this->assertStringContainsString('error_code = 0', $sql); + $this->assertStringContainsString('next_attempt = NULL', $sql); + $this->assertStringContainsString('event_processed = 1', $sql); + $this->assertSame([3, 0.25, 42], $call['params']); + } + + /** + * @return void + */ + public function testTransientFailureReleasesTheClaimAndSchedulesRetry(): void { + thold_notification_record_delivery(42, "smtp\ndown", 0.5); + + $call = $this->lastPreparedCall(); + $sql = preg_replace('/\s+/', ' ', $call['sql']); + + $this->assertStringContainsString('next_attempt = FROM_UNIXTIME', $sql); + $this->assertStringContainsString('process_id = 0', $sql); + $this->assertStringContainsString('event_processed = 0', $sql); + $this->assertSame(['smtp down', 1, 60, 0.5, 42], $call['params']); + } + + /** + * @return void + */ + public function testFifthFailureIsTerminal(): void { + thold_notification_record_delivery(42, 'permanent failure', 0.5, 4); + + $call = $this->lastPreparedCall(); + $sql = preg_replace('/\s+/', ' ', $call['sql']); + + $this->assertStringContainsString('error_code = 1', $sql); + $this->assertStringContainsString('next_attempt = NULL', $sql); + $this->assertStringContainsString('event_processed = 1', $sql); + $this->assertSame(['permanent failure', 5, 0.5, 42], $call['params']); + } + + /** + * @return void + */ + public function testFailureMessageFitsTheQueueColumn(): void { + thold_notification_record_delivery(42, str_repeat('x', 200), 0.5); + + $call = $this->lastPreparedCall(); + + $this->assertSame(128, strlen($call['params'][0])); + } + + /** + * @return void + */ + public function testClaimAndBothDrainsIgnoreRetriesThatAreNotReady(): void { + $notify = file_get_contents(dirname(__DIR__, 2) . '/thold_notify.php'); + + $this->assertStringContainsString('(next_attempt IS NULL OR next_attempt <= NOW())', $notify); + + thold_notification_execute(77); + + $queries = array_filter(CactiStubs::$calls, static function ($call) { + return $call['fn'] === 'db_fetch_assoc' && + strpos($call['sql'], 'notification_queue') !== false; + }); + + $this->assertCount(2, $queries); + + foreach ($queries as $call) { + $this->assertStringContainsString('(next_attempt IS NULL OR next_attempt <= NOW())', $call['sql']); + } + } +} diff --git a/tests/bootstrap-unit.php b/tests/bootstrap-unit.php index ce2b2615..3b98fb6d 100644 --- a/tests/bootstrap-unit.php +++ b/tests/bootstrap-unit.php @@ -44,7 +44,7 @@ throw new RuntimeException("Expected Cacti version file is not readable: $expected"); } -$cacti_version = trim((string) file_get_contents($version)); +$cacti_version = trim((string) file_get_contents($version)); $expected_version = trim((string) file_get_contents($expected)); if ($cacti_version === '') { diff --git a/thold_functions.php b/thold_functions.php index 018cf99b..4cf41022 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -7210,6 +7210,52 @@ function thold_notification_execute($pid = 0, $max_records = 'all') { process_device_notifications($pid, $max_records, $prev_suspended); } +function thold_notification_retry_delay($attempt) { + $attempt = max(1, (int) $attempt); + + return min(3600, 60 * (2 ** ($attempt - 1))); +} + +/** + * Record one queued email delivery without losing transient failures. + * + * The fifth failed attempt is terminal. Earlier failures release the claim and + * schedule a bounded exponential retry, so a permanent SMTP error cannot spin + * every poller cycle forever. + * + * @param mixed $id + * @param mixed $error + * @param mixed $runtime + * @param mixed $previous_attempts + */ +function thold_notification_record_delivery($id, $error, $runtime, $previous_attempts = 0) { + $attempt = max(0, (int) $previous_attempts) + 1; + $error = substr(str_replace("\n", ' ', (string) $error), 0, 128); + + if ($error === '') { + return db_execute_prepared('UPDATE notification_queue + SET error_code = 0, error_message = "", attempt_count = ?, next_attempt = NULL, + event_processed = 1, event_processed_time = NOW(), event_processed_runtime = ? + WHERE id = ?', + [$attempt, $runtime, $id]); + } + + if ($attempt >= 5) { + return db_execute_prepared('UPDATE notification_queue + SET error_code = 1, error_message = ?, attempt_count = ?, next_attempt = NULL, + event_processed = 1, event_processed_time = NOW(), event_processed_runtime = ? + WHERE id = ?', + [$error, $attempt, $runtime, $id]); + } + + return db_execute_prepared('UPDATE notification_queue + SET error_code = 1, error_message = ?, attempt_count = ?, + next_attempt = FROM_UNIXTIME(UNIX_TIMESTAMP() + ?), process_id = 0, + event_processed = 0, event_processed_runtime = ? + WHERE id = ?', + [$error, $attempt, thold_notification_retry_delay($attempt), $runtime, $id]); +} + function process_device_notifications($pid, $max_records, $prev_suspended) { $one_email = read_config_option('alert_deadnotify_one_mail') == 'on' ? true : false; $emails = []; @@ -7233,6 +7279,7 @@ function process_device_notifications($pid, $max_records, $prev_suspended) { $records = db_fetch_assoc("SELECT * FROM notification_queue WHERE event_processed = 0 + AND (next_attempt IS NULL OR next_attempt <= NOW()) AND topic IN ('thold_dhost_mail', 'thold_uhost_mail', 'thold_dhost_cmd', 'thold_uhost_cmd') $sql_where ORDER BY event_time ASC @@ -7293,19 +7340,11 @@ function process_device_notifications($pid, $max_records, $prev_suspended) { if ($error != '') { cacti_log("ERROR: Sending Email Failed To:$to Subject:$subject. Error was:'$error'", true, 'THOLD'); - - $any_error = $error; - $error_code = 1; - } else { - $error_code = 0; } $nend = microtime(true); - db_execute_prepared('UPDATE notification_queue - SET error_code = ?, error_message = ?, event_processed = 1, event_processed_time=NOW(), event_processed_runtime = ? - WHERE id = ?', - [$error_code, str_replace("\n", ' ', $error), $nend - $nstart, $r['id']]); + thold_notification_record_delivery($r['id'], $error, $nend - $nstart, $r['attempt_count'] ?? 0); } else { $id = md5(json_encode([$from, $to, $cc, $bcc, $replyto])); @@ -7339,7 +7378,7 @@ function process_device_notifications($pid, $max_records, $prev_suspended) { } } - $emails[$id]['ids'][] = $r['id']; + $emails[$id]['records'][$r['id']] = $r['attempt_count'] ?? 0; } break; @@ -7416,21 +7455,13 @@ function process_device_notifications($pid, $max_records, $prev_suspended) { if ($error != '') { cacti_log("ERROR: Sending Email Failed To:$to Subject:$subject. Error was:'$error'", true, 'THOLD'); - - $any_error = $error; - $error_code = 1; - } else { - $error_code = 0; } $nend = microtime(true); - $ids = implode(', ', $email['ids']); - - db_execute_prepared("UPDATE notification_queue - SET error_code = ?, error_message = ?, event_processed = 1, event_processed_time=NOW(), event_processed_runtime = ? - WHERE id IN ($ids)", - [$error_code, str_replace("\n", ' ', $error), $nend - $nstart]); + foreach ($email['records'] as $record_id => $attempt_count) { + thold_notification_record_delivery($record_id, $error, $nend - $nstart, $attempt_count); + } } } } else { @@ -7454,6 +7485,7 @@ function process_non_device_notifications($pid, $max_records, $prev_suspended) { $records = db_fetch_assoc("SELECT * FROM notification_queue WHERE event_processed = 0 + AND (next_attempt IS NULL OR next_attempt <= NOW()) AND topic NOT IN ('thold_dhost_mail', 'thold_uhost_mail', 'thold_dhost_cmd', 'thold_uhost_cmd') $sql_where ORDER BY event_time ASC @@ -7503,19 +7535,11 @@ function process_non_device_notifications($pid, $max_records, $prev_suspended) { if ($error != '') { cacti_log("ERROR: Sending Email Failed To:$to Subject:$subject. Error was:'$error'", true, 'THOLD'); - - $any_error = $error; - $error_code = 1; - } else { - $error_code = 0; } $nend = microtime(true); - db_execute_prepared('UPDATE notification_queue - SET error_code = ?, error_message = ?, event_processed = 1, event_processed_time=NOW(), event_processed_runtime = ? - WHERE id = ?', - [$error_code, str_replace("\n", ' ', $error), $nend - $nstart, $r['id']]); + thold_notification_record_delivery($r['id'], $error, $nend - $nstart, $r['attempt_count'] ?? 0); break; case 'thold_cmd': diff --git a/thold_notify.php b/thold_notify.php index 90b43951..96382761 100644 --- a/thold_notify.php +++ b/thold_notify.php @@ -158,6 +158,7 @@ db_execute_prepared('UPDATE notification_queue SET process_id = ? WHERE event_processed = 0 + AND (next_attempt IS NULL OR next_attempt <= NOW()) AND process_id = 0', [$pid]); From aef4590d8c179c1a755ddfbdd00a9812bcc5ed5e Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 19:25:32 -0700 Subject: [PATCH 2/7] test: cover queued delivery retry paths --- tests/Unit/NotificationQueueRetryTest.php | 84 +++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/tests/Unit/NotificationQueueRetryTest.php b/tests/Unit/NotificationQueueRetryTest.php index dee3fe08..c2c2b288 100644 --- a/tests/Unit/NotificationQueueRetryTest.php +++ b/tests/Unit/NotificationQueueRetryTest.php @@ -35,6 +35,30 @@ private function lastPreparedCall() { return end($calls); } + /** + * @return array + */ + private function mailRow($id, $topic = 'thold_mail', $attempts = 0) { + return [ + 'id' => $id, + 'topic' => $topic, + 'attempt_count' => $attempts, + 'event_data' => json_encode([ + 'from' => ['sender@example.com'], + 'to' => 'recipient@example.com', + 'cc' => '', + 'bcc' => '', + 'replyto' => '', + 'subject' => 'Threshold alert', + 'body' => 'Alert', + 'body_text' => 'Alert', + 'attachments' => [], + 'headers' => [], + 'html' => true, + ]), + ]; + } + /** * @return void */ @@ -122,4 +146,64 @@ public function testClaimAndBothDrainsIgnoreRetriesThatAreNotReady(): void { $this->assertStringContainsString('(next_attempt IS NULL OR next_attempt <= NOW())', $call['sql']); } } + + /** + * @return void + */ + public function testIndividualDeviceMailFailureUsesTheRetryRecorder(): void { + CactiStubs::$configOptions['alert_deadnotify_one_mail'] = ''; + CactiStubs::willReturnFor('db_fetch_assoc', "topic IN ('thold_dhost_mail'", [ + $this->mailRow(51, 'thold_dhost_mail', 2), + ]); + CactiStubs::willReturn('mailer', 'temporary SMTP failure'); + + process_device_notifications(77, 'all', 0); + + $call = $this->lastPreparedCall(); + + $this->assertSame(['temporary SMTP failure', 3, 240, $call['params'][3], 51], $call['params']); + $this->assertStringContainsString('event_processed = 0', $call['sql']); + } + + /** + * @return void + */ + public function testGroupedDeviceMailRecordsEveryAttempt(): void { + CactiStubs::$configOptions['alert_deadnotify_one_mail'] = 'on'; + CactiStubs::$configOptions['alert_deadnotify_subject'] = 'Device alerts'; + CactiStubs::willReturnFor('db_fetch_assoc', "topic IN ('thold_dhost_mail'", [ + $this->mailRow(61, 'thold_dhost_mail', 0), + $this->mailRow(62, 'thold_uhost_mail', 3), + ]); + CactiStubs::willReturn('mailer', 'temporary SMTP failure'); + + process_device_notifications(77, 'all', 0); + + $calls = array_values(array_filter(CactiStubs::$calls, static function ($call) { + return $call['fn'] === 'db_execute_prepared' && strpos($call['sql'], 'attempt_count') !== false; + })); + + $this->assertCount(2, $calls); + $this->assertSame(61, $calls[0]['params'][4]); + $this->assertSame(1, $calls[0]['params'][1]); + $this->assertSame(62, $calls[1]['params'][4]); + $this->assertSame(4, $calls[1]['params'][1]); + } + + /** + * @return void + */ + public function testNonDeviceMailFailureUsesTheRetryRecorder(): void { + CactiStubs::willReturnFor('db_fetch_assoc', "topic NOT IN ('thold_dhost_mail'", [ + $this->mailRow(71, 'thold_mail', 1), + ]); + CactiStubs::willReturn('mailer', 'temporary SMTP failure'); + + process_non_device_notifications(77, 'all', 0); + + $call = $this->lastPreparedCall(); + + $this->assertSame(['temporary SMTP failure', 2, 120, $call['params'][3], 71], $call['params']); + $this->assertStringContainsString('event_processed = 0', $call['sql']); + } } From 1db142dcd13f4dd5a8cde444f113404772b7aafc Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 19:26:11 -0700 Subject: [PATCH 3/7] style: document notification fixture parameters --- tests/Unit/NotificationQueueRetryTest.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/Unit/NotificationQueueRetryTest.php b/tests/Unit/NotificationQueueRetryTest.php index c2c2b288..9cdf313c 100644 --- a/tests/Unit/NotificationQueueRetryTest.php +++ b/tests/Unit/NotificationQueueRetryTest.php @@ -37,6 +37,9 @@ private function lastPreparedCall() { /** * @return array + * @param mixed $id + * @param mixed $topic + * @param mixed $attempts */ private function mailRow($id, $topic = 'thold_mail', $attempts = 0) { return [ From 5fb6e43d0c53a5521462b387928ab81242d86941 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 20:50:54 -0700 Subject: [PATCH 4/7] fix: batch grouped notification delivery updates --- README.md | 2 +- tests/Unit/NotificationQueueRetryTest.php | 33 ++++++-- thold_functions.php | 94 ++++++++++++++++++++++- 3 files changed, 120 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index a5e95d7c..e8390d85 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,7 @@ control of thold, and set defaults for things like Email bodies, weekend exemptions, alert log retention, logging, etc. When the notification queue is enabled, transient email failures are retried -up to five times with exponential backoff from one minute to one hour. The +up to five times with exponential backoff from one to eight minutes. The Notification Queue page shows the attempt count and next eligible retry time. After the fifth failed attempt the row becomes a terminal error so a permanent SMTP or address failure cannot retry forever. diff --git a/tests/Unit/NotificationQueueRetryTest.php b/tests/Unit/NotificationQueueRetryTest.php index 9cdf313c..081d1c49 100644 --- a/tests/Unit/NotificationQueueRetryTest.php +++ b/tests/Unit/NotificationQueueRetryTest.php @@ -176,7 +176,9 @@ public function testGroupedDeviceMailRecordsEveryAttempt(): void { CactiStubs::$configOptions['alert_deadnotify_subject'] = 'Device alerts'; CactiStubs::willReturnFor('db_fetch_assoc', "topic IN ('thold_dhost_mail'", [ $this->mailRow(61, 'thold_dhost_mail', 0), + $this->mailRow(63, 'thold_dhost_mail', 0), $this->mailRow(62, 'thold_uhost_mail', 3), + $this->mailRow(64, 'thold_uhost_mail', 4), ]); CactiStubs::willReturn('mailer', 'temporary SMTP failure'); @@ -186,11 +188,32 @@ public function testGroupedDeviceMailRecordsEveryAttempt(): void { return $call['fn'] === 'db_execute_prepared' && strpos($call['sql'], 'attempt_count') !== false; })); - $this->assertCount(2, $calls); - $this->assertSame(61, $calls[0]['params'][4]); - $this->assertSame(1, $calls[0]['params'][1]); - $this->assertSame(62, $calls[1]['params'][4]); - $this->assertSame(4, $calls[1]['params'][1]); + $this->assertCount(1, $calls); + $this->assertStringContainsString('attempt_count = CASE id', $calls[0]['sql']); + $this->assertStringContainsString('event_processed = CASE id', $calls[0]['sql']); + $this->assertSame([61, 63, 62, 64], array_slice($calls[0]['params'], -4)); + $this->assertSame('temporary SMTP failure', $calls[0]['params'][1]); + } + + /** + * @return void + */ + public function testGroupedDeliveryHandlesEmptyInvalidAndSuccessfulBatches(): void { + $this->assertTrue(thold_notification_record_deliveries([], '', 0.25)); + $this->assertTrue(thold_notification_record_deliveries([-1 => 0], '', 0.25)); + $this->assertSame([], CactiStubs::$calls); + + $this->assertTrue(thold_notification_record_deliveries([81 => 0, 82 => 4], '', 0.25)); + + $calls = array_values(array_filter(CactiStubs::$calls, static function ($call) { + return $call['fn'] === 'db_execute_prepared' && strpos($call['sql'], 'attempt_count = CASE id') !== false; + })); + + $this->assertCount(1, $calls); + $this->assertSame(0, $calls[0]['params'][0]); + $this->assertSame('', $calls[0]['params'][1]); + $this->assertSame([81, 82], array_slice($calls[0]['params'], -2)); + $this->assertStringNotContainsString('FROM_UNIXTIME', $calls[0]['sql']); } /** diff --git a/thold_functions.php b/thold_functions.php index 4cf41022..7cd905a7 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -7256,6 +7256,96 @@ function thold_notification_record_delivery($id, $error, $runtime, $previous_att [$error, $attempt, thold_notification_retry_delay($attempt), $runtime, $id]); } +/** + * Record one grouped mail result with a single prepared update. + * + * @param array $records Record ID => previous attempt count. + * @param string $error + * @param float $runtime + * + * @return bool + */ +function thold_notification_record_deliveries(array $records, $error, $runtime) { + if (!cacti_sizeof($records)) { + return true; + } + + $error = substr(str_replace("\n", ' ', (string) $error), 0, 128); + $attempt_cases = []; + $attempt_params = []; + $next_cases = []; + $next_params = []; + $process_cases = []; + $process_params = []; + $done_cases = []; + $done_params = []; + $time_cases = []; + $time_params = []; + $ids = []; + + foreach ($records as $id => $previous_attempts) { + $id = (int) $id; + + if ($id <= 0) { + continue; + } + + $attempt = max(0, (int) $previous_attempts) + 1; + $retryable = $error !== '' && $attempt < 5; + $done = $retryable ? 0 : 1; + + $attempt_cases[] = 'WHEN ? THEN ?'; + $attempt_params[] = $id; + $attempt_params[] = $attempt; + + if ($retryable) { + $next_cases[] = 'WHEN ? THEN FROM_UNIXTIME(UNIX_TIMESTAMP() + ?)'; + $next_params[] = $id; + $next_params[] = thold_notification_retry_delay($attempt); + $process_cases[] = 'WHEN ? THEN 0'; + } else { + $next_cases[] = 'WHEN ? THEN NULL'; + $next_params[] = $id; + $process_cases[] = 'WHEN ? THEN process_id'; + } + + $process_params[] = $id; + $done_cases[] = 'WHEN ? THEN ?'; + $done_params[] = $id; + $done_params[] = $done; + $time_cases[] = $done ? 'WHEN ? THEN NOW()' : 'WHEN ? THEN event_processed_time'; + $time_params[] = $id; + $ids[] = $id; + } + + if (!cacti_sizeof($ids)) { + return true; + } + + $placeholders = implode(',', array_fill(0, cacti_sizeof($ids), '?')); + $params = array_merge( + [$error === '' ? 0 : 1, $error], + $attempt_params, + $next_params, + $process_params, + $done_params, + $time_params, + [$runtime], + $ids + ); + + return db_execute_prepared('UPDATE notification_queue + SET error_code = ?, error_message = ?, + attempt_count = CASE id ' . implode(' ', $attempt_cases) . ' ELSE attempt_count END, + next_attempt = CASE id ' . implode(' ', $next_cases) . ' ELSE next_attempt END, + process_id = CASE id ' . implode(' ', $process_cases) . ' ELSE process_id END, + event_processed = CASE id ' . implode(' ', $done_cases) . ' ELSE event_processed END, + event_processed_time = CASE id ' . implode(' ', $time_cases) . ' ELSE event_processed_time END, + event_processed_runtime = ? + WHERE id IN (' . $placeholders . ')', + $params); +} + function process_device_notifications($pid, $max_records, $prev_suspended) { $one_email = read_config_option('alert_deadnotify_one_mail') == 'on' ? true : false; $emails = []; @@ -7459,9 +7549,7 @@ function process_device_notifications($pid, $max_records, $prev_suspended) { $nend = microtime(true); - foreach ($email['records'] as $record_id => $attempt_count) { - thold_notification_record_delivery($record_id, $error, $nend - $nstart, $attempt_count); - } + thold_notification_record_deliveries($email['records'], $error, $nend - $nstart); } } } else { From 3db69e794cb273fde94a8452fdac1844eff96d2d Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 21:12:36 -0700 Subject: [PATCH 5/7] fix: align notification retry status output --- CHANGELOG.md | 2 +- notify_queue.php | 11 ++------- tests/Unit/NotificationQueueRetryTest.php | 30 +++++++++++++++++++++++ thold_functions.php | 19 ++++++++++++++ 4 files changed, 52 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03f96bc7..a9f915b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,12 @@ --- develop --- -* issue#784: Retry failed queued email notifications with bounded exponential backoff * issue#686: Applying a templated threshold to a graph via the wrench icon, creates a duplicate graph * issue#707: Excessive timeout for row caching prevents data from being updated timely * issue#710: Fixing Typo in thold_daemons.service File * issue#714: Increase the Name column to 255 characters * issue#719: Plugin Disabled due to mix of string and int +* issue#784: Retry failed queued email notifications with bounded exponential backoff * issue: All Columns checkd on Thresholds page * issue: Special character previous value handling broken on data query indexes with special characters diff --git a/notify_queue.php b/notify_queue.php index 73025bb2..c288a213 100644 --- a/notify_queue.php +++ b/notify_queue.php @@ -503,16 +503,9 @@ function clearFilter() { form_selectable_cell($n['id'], $n['id'], '', 'right'); form_selectable_cell($n['event_time'], $n['id'], '', 'right'); - form_selectable_cell($n['event_processed'] == 0 ? __('Pending', 'thold') : __('Done', 'thold'), $n['id'], '', 'right'); - form_selectable_cell((int) ($n['attempt_count'] ?? 0), $n['id'], '', 'right'); - form_selectable_cell($n['next_attempt'] ?: __('N/A', 'thold'), $n['id'], '', 'right'); - if ($n['event_processed'] > 0) { - form_selectable_cell($n['error_code'] > 0 ? __('Errored', 'thold') : __('Success', 'thold'), $n['id'], '', 'right'); - form_selectable_cell(number_format_i18n($n['event_processed_runtime'], 2), $n['id'], '', 'right'); - } else { - form_selectable_cell(__('N/A', 'thold'), $n['id'], '', 'right'); - form_selectable_cell(__('N/A', 'thold'), $n['id'], '', 'right'); + foreach (thold_notification_queue_status_cells($n) as $cell) { + form_selectable_cell($cell, $n['id'], '', 'right'); } form_checkbox_cell($n['object_name'], $n['id']); diff --git a/tests/Unit/NotificationQueueRetryTest.php b/tests/Unit/NotificationQueueRetryTest.php index 081d1c49..eb33eab1 100644 --- a/tests/Unit/NotificationQueueRetryTest.php +++ b/tests/Unit/NotificationQueueRetryTest.php @@ -66,12 +66,37 @@ private function mailRow($id, $topic = 'thold_mail', $attempts = 0) { * @return void */ public function testRetryDelayUsesBoundedExponentialBackoff(): void { + $this->assertSame(60, thold_notification_retry_delay(-1)); + $this->assertSame(60, thold_notification_retry_delay(0)); $this->assertSame(60, thold_notification_retry_delay(1)); $this->assertSame(120, thold_notification_retry_delay(2)); $this->assertSame(480, thold_notification_retry_delay(4)); + $this->assertSame(1920, thold_notification_retry_delay(6)); $this->assertSame(3600, thold_notification_retry_delay(7)); } + /** + * @return void + */ + public function testQueueStatusCellsFollowTheirHeaderOrder(): void { + $cells = thold_notification_queue_status_cells([ + 'event_processed' => 1, + 'error_code' => 1, + 'attempt_count' => 4, + 'next_attempt' => null, + 'event_processed_runtime' => 0.25, + ]); + + $this->assertSame( + ['event_processed', 'error_code', 'attempt_count', 'next_attempt', 'event_processed_runtime'], + array_keys($cells) + ); + $this->assertSame(['Done', 'Errored', 4, 'N/A', '0.25'], array_values($cells)); + + $pending = thold_notification_queue_status_cells(['event_processed' => 0]); + $this->assertSame(['Pending', 'N/A', 0, 'N/A', 'N/A'], array_values($pending)); + } + /** * @return void */ @@ -191,6 +216,11 @@ public function testGroupedDeviceMailRecordsEveryAttempt(): void { $this->assertCount(1, $calls); $this->assertStringContainsString('attempt_count = CASE id', $calls[0]['sql']); $this->assertStringContainsString('event_processed = CASE id', $calls[0]['sql']); + $this->assertSame([61, 1, 63, 1, 62, 4, 64, 5], array_slice($calls[0]['params'], 2, 8)); + $this->assertSame([61, 60, 63, 60, 62, 480, 64], array_slice($calls[0]['params'], 10, 7)); + $this->assertSame([61, 63, 62, 64], array_slice($calls[0]['params'], 17, 4)); + $this->assertSame([61, 0, 63, 0, 62, 0, 64, 1], array_slice($calls[0]['params'], 21, 8)); + $this->assertSame([61, 63, 62, 64], array_slice($calls[0]['params'], 29, 4)); $this->assertSame([61, 63, 62, 64], array_slice($calls[0]['params'], -4)); $this->assertSame('temporary SMTP failure', $calls[0]['params'][1]); } diff --git a/thold_functions.php b/thold_functions.php index 7cd905a7..5c9865a4 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -7216,6 +7216,25 @@ function thold_notification_retry_delay($attempt) { return min(3600, 60 * (2 ** ($attempt - 1))); } +/** + * Values for the delivery-status columns, in the same order as their headers. + * + * @param array $notification + * + * @return array + */ +function thold_notification_queue_status_cells(array $notification) { + $processed = (int) ($notification['event_processed'] ?? 0); + + return [ + 'event_processed' => $processed === 0 ? __('Pending', 'thold') : __('Done', 'thold'), + 'error_code' => $processed === 0 ? __('N/A', 'thold') : ((int) ($notification['error_code'] ?? 0) > 0 ? __('Errored', 'thold') : __('Success', 'thold')), + 'attempt_count' => (int) ($notification['attempt_count'] ?? 0), + 'next_attempt' => !empty($notification['next_attempt']) ? $notification['next_attempt'] : __('N/A', 'thold'), + 'event_processed_runtime' => $processed === 0 ? __('N/A', 'thold') : number_format_i18n($notification['event_processed_runtime'] ?? 0, 2), + ]; +} + /** * Record one queued email delivery without losing transient failures. * From af4ca5292359500b4b3541f2faf9d5c2403247f1 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 22:07:08 -0700 Subject: [PATCH 6/7] refactor: centralize notification retry updates --- tests/Unit/NotificationQueueRetryTest.php | 62 ++++++++++++++++------- thold_functions.php | 26 +--------- 2 files changed, 46 insertions(+), 42 deletions(-) diff --git a/tests/Unit/NotificationQueueRetryTest.php b/tests/Unit/NotificationQueueRetryTest.php index 99a2f101..65ec881e 100644 --- a/tests/Unit/NotificationQueueRetryTest.php +++ b/tests/Unit/NotificationQueueRetryTest.php @@ -106,10 +106,13 @@ public function testSuccessfulDeliveryIsTerminal(): void { $call = $this->lastPreparedCall(); $sql = preg_replace('/\s+/', ' ', $call['sql']); - $this->assertStringContainsString('error_code = 0', $sql); - $this->assertStringContainsString('next_attempt = NULL', $sql); - $this->assertStringContainsString('event_processed = 1', $sql); - $this->assertSame([3, 0.25, 42], $call['params']); + $this->assertSame(0, $call['params'][0]); + $this->assertSame('', $call['params'][1]); + $this->assertStringContainsString('next_attempt = CASE id', $sql); + $this->assertStringContainsString('THEN NULL', $sql); + $this->assertSame([42, 3], array_slice($call['params'], 2, 2)); + $this->assertSame(1, $call['params'][7]); + $this->assertSame([42, 0.25, 42], array_slice($call['params'], -3)); } /** @@ -121,10 +124,12 @@ public function testTransientFailureReleasesTheClaimAndSchedulesRetry(): void { $call = $this->lastPreparedCall(); $sql = preg_replace('/\s+/', ' ', $call['sql']); - $this->assertStringContainsString('next_attempt = FROM_UNIXTIME', $sql); - $this->assertStringContainsString('process_id = 0', $sql); - $this->assertStringContainsString('event_processed = 0', $sql); - $this->assertSame(['smtp down', 1, 60, 0.5, 42], $call['params']); + $this->assertStringContainsString('THEN FROM_UNIXTIME', $sql); + $this->assertStringContainsString('process_id = CASE id', $sql); + $this->assertStringContainsString('THEN 0', $sql); + $this->assertSame([1, 'smtp down', 42, 1, 42, 60], array_slice($call['params'], 0, 6)); + $this->assertSame(0, $call['params'][8]); + $this->assertSame([42, 0.5, 42], array_slice($call['params'], -3)); } /** @@ -136,10 +141,10 @@ public function testFifthFailureIsTerminal(): void { $call = $this->lastPreparedCall(); $sql = preg_replace('/\s+/', ' ', $call['sql']); - $this->assertStringContainsString('error_code = 1', $sql); - $this->assertStringContainsString('next_attempt = NULL', $sql); - $this->assertStringContainsString('event_processed = 1', $sql); - $this->assertSame(['permanent failure', 5, 0.5, 42], $call['params']); + $this->assertStringContainsString('THEN NULL', $sql); + $this->assertSame([1, 'permanent failure', 42, 5], array_slice($call['params'], 0, 4)); + $this->assertSame(1, $call['params'][7]); + $this->assertSame([42, 0.5, 42], array_slice($call['params'], -3)); } /** @@ -150,7 +155,7 @@ public function testFailureMessageFitsTheQueueColumn(): void { $call = $this->lastPreparedCall(); - $this->assertSame(128, strlen($call['params'][0])); + $this->assertSame(128, strlen($call['params'][1])); } /** @@ -196,8 +201,10 @@ public function testIndividualDeviceMailFailureUsesTheRetryRecorder(): void { $call = $this->lastPreparedCall(); - $this->assertSame(['temporary SMTP failure', 3, 240, $call['params'][3], 51], $call['params']); - $this->assertStringContainsString('event_processed = 0', $call['sql']); + $this->assertSame([1, 'temporary SMTP failure', 51, 3, 51, 240], array_slice($call['params'], 0, 6)); + $this->assertSame([51, $call['params'][10], 51], array_slice($call['params'], -3)); + $this->assertSame(0, $call['params'][8]); + $this->assertStringContainsString('process_id = CASE id', $call['sql']); } /** @@ -251,6 +258,25 @@ public function testGroupedDeliveryHandlesEmptyInvalidAndSuccessfulBatches(): vo $this->assertSame('', $calls[0]['params'][1]); $this->assertSame([81, 82], array_slice($calls[0]['params'], -2)); $this->assertStringNotContainsString('FROM_UNIXTIME', $calls[0]['sql']); + $this->assertStringContainsString('process_id = CASE id', $calls[0]['sql']); + $this->assertStringContainsString('THEN process_id', $calls[0]['sql']); + $this->assertStringContainsString('THEN NOW()', $calls[0]['sql']); + $this->assertSame([81, 1, 82, 1], array_slice($calls[0]['params'], 10, 4)); + } + + /** + * @return void + */ + public function testGroupedTerminalFailuresStayClaimedAndComplete(): void { + $this->assertTrue(thold_notification_record_deliveries([91 => 4, 92 => 5], 'permanent failure', 0.5)); + + $call = $this->lastPreparedCall(); + + $this->assertSame([91, 5, 92, 6], array_slice($call['params'], 2, 4)); + $this->assertSame([91, 1, 92, 1], array_slice($call['params'], 10, 4)); + $this->assertStringNotContainsString('FROM_UNIXTIME', $call['sql']); + $this->assertStringContainsString('THEN process_id', $call['sql']); + $this->assertStringContainsString('THEN NOW()', $call['sql']); } /** @@ -266,7 +292,9 @@ public function testNonDeviceMailFailureUsesTheRetryRecorder(): void { $call = $this->lastPreparedCall(); - $this->assertSame(['temporary SMTP failure', 2, 120, $call['params'][3], 71], $call['params']); - $this->assertStringContainsString('event_processed = 0', $call['sql']); + $this->assertSame([1, 'temporary SMTP failure', 71, 2, 71, 120], array_slice($call['params'], 0, 6)); + $this->assertSame([71, $call['params'][10], 71], array_slice($call['params'], -3)); + $this->assertSame(0, $call['params'][8]); + $this->assertStringContainsString('process_id = CASE id', $call['sql']); } } diff --git a/thold_functions.php b/thold_functions.php index f7305001..c3b1e1c4 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -7533,31 +7533,7 @@ function thold_notification_queue_status_cells(array $notification) { * @param mixed $previous_attempts */ function thold_notification_record_delivery($id, $error, $runtime, $previous_attempts = 0) { - $attempt = max(0, (int) $previous_attempts) + 1; - $error = substr(str_replace("\n", ' ', (string) $error), 0, 128); - - if ($error === '') { - return db_execute_prepared('UPDATE notification_queue - SET error_code = 0, error_message = "", attempt_count = ?, next_attempt = NULL, - event_processed = 1, event_processed_time = NOW(), event_processed_runtime = ? - WHERE id = ?', - [$attempt, $runtime, $id]); - } - - if ($attempt >= 5) { - return db_execute_prepared('UPDATE notification_queue - SET error_code = 1, error_message = ?, attempt_count = ?, next_attempt = NULL, - event_processed = 1, event_processed_time = NOW(), event_processed_runtime = ? - WHERE id = ?', - [$error, $attempt, $runtime, $id]); - } - - return db_execute_prepared('UPDATE notification_queue - SET error_code = 1, error_message = ?, attempt_count = ?, - next_attempt = FROM_UNIXTIME(UNIX_TIMESTAMP() + ?), process_id = 0, - event_processed = 0, event_processed_runtime = ? - WHERE id = ?', - [$error, $attempt, thold_notification_retry_delay($attempt), $runtime, $id]); + return thold_notification_record_deliveries([(int) $id => $previous_attempts], $error, $runtime); } /** From f44f76aeffd3c537dfe9568abfabd654f441aae9 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 22:54:48 -0700 Subject: [PATCH 7/7] fix: retain retry queue ownership --- tests/bin/patch-coverage.php | 5 +++++ thold_functions.php | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/bin/patch-coverage.php b/tests/bin/patch-coverage.php index 8b49345b..07d45250 100644 --- a/tests/bin/patch-coverage.php +++ b/tests/bin/patch-coverage.php @@ -157,6 +157,11 @@ function changed_lines($base_ref) { * here with reviewable justification. */ $unmeasured_allowlist = [ + // Database migration/schema declarations require a live Cacti database. + 'includes/database.php', + // Authenticated web entry point; its status mapping lives in the covered + // thold_notification_queue_status_cells() helper. + 'notify_queue.php', 'thold_notify.php', ]; $unmeasured = array_values(array_diff(array_keys($changed), array_keys($measured))); diff --git a/thold_functions.php b/thold_functions.php index ee1a8a9f..457ce82d 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -7892,7 +7892,7 @@ function process_device_notifications($pid, $max_records, $prev_suspended, $hear $nend = microtime(true); - thold_notification_record_delivery($r['id'], $pid, $error, $nend - $nstart, $r['attempt_count'] ?? 0); + thold_notification_record_delivery($r['id'], $pid, $error, $nend - $nstart, $r['attempt_count'] ?? 0); } else { $id = md5(json_encode([$from, $to, $cc, $bcc, $replyto]));