From 7646a018f9cedc7b70f891ee9871b65c5d5baad5 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 22:21:51 -0700 Subject: [PATCH 01/17] fix: preserve counter sample time pairs --- CHANGELOG.md | 1 + README.md | 4 +++ tests/Unit/TholdGetCurrentvalTest.php | 38 +++++++++++++++++++++++++++ thold_functions.php | 22 ++++++++++++++++ thold_process.php | 10 ++----- 5 files changed, 67 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b7db7d7..42b97ce9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * 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#815: Keep counter sample values and timestamps synchronized * issue: All Columns checkd on Thresholds page * issue: Special character previous value handling broken on data query indexes with special characters diff --git a/README.md b/README.md index 46310dce..7dbae9ee 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,10 @@ 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. +Counter thresholds preserve the previous value and timestamp together when a +poll has no numeric sample, preventing the next rate from using mismatched +interval data. + 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/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index 4c1472cf..56479579 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -173,4 +173,42 @@ public function testMissingDataSourceYieldsTheNoValueSentinel(): void { $this->assertSame('', thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime)); } + + /** + * @return void + */ + public function testMissedSampleCarriesTheValueAndTimestampTogether(): void { + $thold = $this->threshold(['lasttime' => 1000, 'oldvalue' => 100]); + + $missed = thold_sample_persistence($thold, [], 1300); + $this->assertSame(['lasttime' => 1000, 'oldvalue' => 100], $missed); + $this->assertSame($missed, thold_sample_persistence($thold, ['traffic_in' => 'U'], 1300)); + $this->assertSame( + ['lasttime' => 1600, 'oldvalue' => 700], + thold_sample_persistence($thold, ['traffic_in' => 700], 1600) + ); + + $thold['lasttime'] = $missed['lasttime']; + $thold['oldvalue'] = $missed['oldvalue']; + $reindexed = [4 => ['traffic_in' => 700]]; + $time_reindexed = [4 => 1600]; + $item = []; + $currenttime = 0; + + $this->assertEqualsWithDelta( + 1, + thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime), + 1.0e-9 + ); + } + + /** + * @return void + */ + public function testDaemonPersistsTheSamplePairHelperResult(): void { + $source = file_get_contents(dirname(__DIR__, 2) . '/thold_process.php'); + + $this->assertStringContainsString('thold_sample_persistence($thold_data, $item, $currenttime)', $source); + $this->assertStringContainsString("\$sample['lasttime'], \$sample['oldvalue']", $source); + } } diff --git a/thold_functions.php b/thold_functions.php index f195d302..7352249e 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -803,6 +803,28 @@ function thold_counter_wrap_delta($oldvalue, $newvalue) { return (4294967296 - $oldvalue) + $newvalue; } +/** + * Persist a raw sample and its timestamp as one causal pair. + * + * @param array $thold_data + * @param array $item + * @param int $currenttime + * + * @return array{lasttime:mixed,oldvalue:mixed} + */ +function thold_sample_persistence(array $thold_data, array $item, $currenttime) { + $name = $thold_data['name']; + + if (isset($item[$name]) && is_numeric($item[$name])) { + return ['lasttime' => $currenttime, 'oldvalue' => $item[$name]]; + } + + return [ + 'lasttime' => $thold_data['lasttime'], + 'oldvalue' => $thold_data['oldvalue'], + ]; +} + function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexed, &$item, &$currenttime) { // adjust the polling interval by the last read, if applicable $currenttime = $rrd_time_reindexed[$thold_data['local_data_id']]; diff --git a/thold_process.php b/thold_process.php index 40078c24..c69454c0 100644 --- a/thold_process.php +++ b/thold_process.php @@ -207,13 +207,7 @@ $currentval = ''; } - // Carry the previous value forward when this cycle has no reading; - // storing a timestamp here corrupts the next delta calculation. - if (isset($item[$thold_data['name']])) { - $rawvalue = $item[$thold_data['name']]; - } else { - $rawvalue = $thold_data['oldvalue']; - } + $sample = thold_sample_persistence($thold_data, $item, $currenttime); thold_daemon_debug(sprintf('Checked Name:%s, Graph:%s, Value:%s, Time:%s', $thold_data['thold_name'], $thold_data['local_graph_id'], $currentval, $currenttime), $thread); @@ -221,7 +215,7 @@ SET tcheck = 1, lastread = ?, lasttime = FROM_UNIXTIME(?), oldvalue = ? WHERE id = ?', - [$currentval, $currenttime, $rawvalue, $thold_data['thold_id']] + [$currentval, $sample['lasttime'], $sample['oldvalue'], $thold_data['thold_id']] ); } From 6dcdfcc36400a26c46c5e2a4fbbde8faa3b81482 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 22:58:35 -0700 Subject: [PATCH 02/17] fix: preserve samples in daemon and poller paths --- includes/polling.php | 19 ++++---- tests/Unit/TholdGetCurrentvalTest.php | 62 +++++++++++++++++++++++++-- thold_functions.php | 39 +++++++++++++++-- thold_process.php | 9 +--- 4 files changed, 104 insertions(+), 25 deletions(-) diff --git a/includes/polling.php b/includes/polling.php index c5c2c1d6..6d18a0c1 100644 --- a/includes/polling.php +++ b/includes/polling.php @@ -206,17 +206,18 @@ function thold_poller_output(&$rrd_update_array) { } } - // This stores the raw value into the data source and is important for - // Counters, where calculating the difference is important. - // The unset case is problematic and may lead to false triggering - // events. So, in those cases, we will store the 'oldvalue'. - if (isset($item[$thold_data['name']])) { - $rawvalue = $item[$thold_data['name']]; + $sample = thold_sample_persistence($thold_data, $item, $currenttime); + + if ($sample['lasttime'] > 0) { + $sql[] = '(' . $thold_data['id'] . ', 1, ' . db_qstr($currentval) . ', FROM_UNIXTIME(' . $sample['lasttime'] . '), ' . db_qstr($sample['oldvalue']) . ')'; } else { - $rawvalue = $thold_data['oldvalue']; + // A never-sampled threshold has the zero-date schema default. Keep + // that pair untouched instead of writing FROM_UNIXTIME(0). + db_execute_prepared('UPDATE thold_data + SET tcheck = 1, lastread = ? + WHERE id = ?', + [$currentval, $thold_data['id']]); } - - $sql[] = '(' . $thold_data['id'] . ', 1, ' . db_qstr($currentval) . ', FROM_UNIXTIME(' . $currenttime . '), ' . db_qstr($rawvalue) . ')'; } if (cacti_sizeof($sql)) { diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index 56479579..7692dbdf 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -38,6 +38,7 @@ public static function setUpBeforeClass(): void { */ private function threshold(array $overrides = []) { return $overrides + [ + 'thold_id' => 9, 'local_data_id' => 4, 'name' => 'traffic_in', 'data_source_type_id' => self::COUNTER, @@ -183,10 +184,16 @@ public function testMissedSampleCarriesTheValueAndTimestampTogether(): void { $missed = thold_sample_persistence($thold, [], 1300); $this->assertSame(['lasttime' => 1000, 'oldvalue' => 100], $missed); $this->assertSame($missed, thold_sample_persistence($thold, ['traffic_in' => 'U'], 1300)); + $this->assertSame($missed, thold_sample_persistence($thold, ['traffic_in' => 'nan'], 1300)); + $this->assertSame($missed, thold_sample_persistence($thold, ['traffic_in' => ''], 1300)); $this->assertSame( ['lasttime' => 1600, 'oldvalue' => 700], thold_sample_persistence($thold, ['traffic_in' => 700], 1600) ); + $this->assertSame( + ['lasttime' => 1600, 'oldvalue' => '700'], + thold_sample_persistence($thold, ['traffic_in' => '700'], 1600) + ); $thold['lasttime'] = $missed['lasttime']; $thold['oldvalue'] = $missed['oldvalue']; @@ -205,10 +212,57 @@ public function testMissedSampleCarriesTheValueAndTimestampTogether(): void { /** * @return void */ - public function testDaemonPersistsTheSamplePairHelperResult(): void { - $source = file_get_contents(dirname(__DIR__, 2) . '/thold_process.php'); + public function testDaemonPersistsThePairWithBoundParameters(): void { + $thold = $this->threshold(['lasttime' => 1000, 'oldvalue' => 100]); + + $this->assertTrue(thold_daemon_persist_sample($thold, [], '', 1300)); + $call = end(CactiStubs::$calls); + $this->assertStringContainsString('lasttime = FROM_UNIXTIME(?)', $call['sql']); + $this->assertSame(['', 1000, 100, 9], $call['params']); + + CactiStubs::reset(); + $this->assertTrue(thold_daemon_persist_sample($thold, ['traffic_in' => 700], 2, 1600)); + $call = end(CactiStubs::$calls); + $this->assertSame([2, 1600, 700, 9], $call['params']); + } + + /** + * @return void + */ + public function testNeverSampledThresholdLeavesTheTimestampPairUntouched(): void { + $thold = $this->threshold(['lasttime' => 0, 'oldvalue' => null]); + + $this->assertSame( + ['lasttime' => 0, 'oldvalue' => null], + thold_sample_persistence($thold, ['traffic_in' => 'U'], 1300) + ); + $this->assertTrue(thold_daemon_persist_sample($thold, ['traffic_in' => 'U'], '', 1300)); + $call = end(CactiStubs::$calls); + $this->assertStringNotContainsString('FROM_UNIXTIME', $call['sql']); + $this->assertStringNotContainsString('oldvalue', $call['sql']); + $this->assertSame(['', 9], $call['params']); + } + + /** + * @return void + */ + public function testMissingPersistenceKeysFailClosed(): void { + $this->assertSame( + ['lasttime' => 0, 'oldvalue' => null], + thold_sample_persistence([], ['traffic_in' => 700], 1600) + ); + } - $this->assertStringContainsString('thold_sample_persistence($thold_data, $item, $currenttime)', $source); - $this->assertStringContainsString("\$sample['lasttime'], \$sample['oldvalue']", $source); + /** + * @return void + */ + public function testDaemonAndPollerBothUseTheSharedPairPolicy(): void { + $daemon = file_get_contents(dirname(__DIR__, 2) . '/thold_process.php'); + $poller = file_get_contents(dirname(__DIR__, 2) . '/includes/polling.php'); + + $this->assertStringContainsString('thold_daemon_persist_sample($thold_data, $item, $currentval, $currenttime)', $daemon); + $this->assertStringContainsString('thold_sample_persistence($thold_data, $item, $currenttime)', $poller); + $this->assertStringContainsString("\$sample['lasttime']", $poller); + $this->assertStringContainsString("\$sample['oldvalue']", $poller); } } diff --git a/thold_functions.php b/thold_functions.php index 7352249e..7ee1f4fe 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -806,6 +806,9 @@ function thold_counter_wrap_delta($oldvalue, $newvalue) { /** * Persist a raw sample and its timestamp as one causal pair. * + * `$thold_data` must provide `name`, `lasttime`, and `oldvalue`; absent values + * fail closed to an unavailable prior sample. + * * @param array $thold_data * @param array $item * @param int $currenttime @@ -813,18 +816,46 @@ function thold_counter_wrap_delta($oldvalue, $newvalue) { * @return array{lasttime:mixed,oldvalue:mixed} */ function thold_sample_persistence(array $thold_data, array $item, $currenttime) { - $name = $thold_data['name']; + $name = (string) ($thold_data['name'] ?? ''); + $currenttime = (int) $currenttime; - if (isset($item[$name]) && is_numeric($item[$name])) { + if ($name !== '' && $currenttime > 0 && isset($item[$name]) && is_numeric($item[$name])) { return ['lasttime' => $currenttime, 'oldvalue' => $item[$name]]; } return [ - 'lasttime' => $thold_data['lasttime'], - 'oldvalue' => $thold_data['oldvalue'], + 'lasttime' => (int) ($thold_data['lasttime'] ?? 0), + 'oldvalue' => $thold_data['oldvalue'] ?? null, ]; } +/** + * Persist one daemon sample without manufacturing a zero SQL timestamp. + * + * @param array $thold_data + * @param array $item + * @param mixed $currentval + * @param int $currenttime + * + * @return bool + */ +function thold_daemon_persist_sample(array $thold_data, array $item, $currentval, $currenttime) { + $sample = thold_sample_persistence($thold_data, $item, $currenttime); + + if ($sample['lasttime'] <= 0) { + return db_execute_prepared('UPDATE thold_data + SET tcheck = 1, lastread = ? + WHERE id = ?', + [$currentval, $thold_data['thold_id']]); + } + + return db_execute_prepared('UPDATE thold_data + SET tcheck = 1, lastread = ?, + lasttime = FROM_UNIXTIME(?), oldvalue = ? + WHERE id = ?', + [$currentval, $sample['lasttime'], $sample['oldvalue'], $thold_data['thold_id']]); +} + function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexed, &$item, &$currenttime) { // adjust the polling interval by the last read, if applicable $currenttime = $rrd_time_reindexed[$thold_data['local_data_id']]; diff --git a/thold_process.php b/thold_process.php index c69454c0..4422b4e7 100644 --- a/thold_process.php +++ b/thold_process.php @@ -207,16 +207,9 @@ $currentval = ''; } - $sample = thold_sample_persistence($thold_data, $item, $currenttime); - thold_daemon_debug(sprintf('Checked Name:%s, Graph:%s, Value:%s, Time:%s', $thold_data['thold_name'], $thold_data['local_graph_id'], $currentval, $currenttime), $thread); - db_execute_prepared('UPDATE thold_data - SET tcheck = 1, lastread = ?, - lasttime = FROM_UNIXTIME(?), oldvalue = ? - WHERE id = ?', - [$currentval, $sample['lasttime'], $sample['oldvalue'], $thold_data['thold_id']] - ); + thold_daemon_persist_sample($thold_data, $item, $currentval, $currenttime); } $tholds = thold_get_thresholds_tholdcheck($thread, $start_time); From bd63f5fc33818470aa31750af4c85eafea9472fa Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 22:59:27 -0700 Subject: [PATCH 03/17] test: exercise poller sample persistence --- includes/polling.php | 13 +++---------- tests/Unit/TholdGetCurrentvalTest.php | 27 ++++++++++++++++++++++++--- thold_functions.php | 26 ++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 13 deletions(-) diff --git a/includes/polling.php b/includes/polling.php index 6d18a0c1..03a46bbc 100644 --- a/includes/polling.php +++ b/includes/polling.php @@ -206,17 +206,10 @@ function thold_poller_output(&$rrd_update_array) { } } - $sample = thold_sample_persistence($thold_data, $item, $currenttime); + $sample_row = thold_polling_sample_row($thold_data, $item, $currentval, $currenttime); - if ($sample['lasttime'] > 0) { - $sql[] = '(' . $thold_data['id'] . ', 1, ' . db_qstr($currentval) . ', FROM_UNIXTIME(' . $sample['lasttime'] . '), ' . db_qstr($sample['oldvalue']) . ')'; - } else { - // A never-sampled threshold has the zero-date schema default. Keep - // that pair untouched instead of writing FROM_UNIXTIME(0). - db_execute_prepared('UPDATE thold_data - SET tcheck = 1, lastread = ? - WHERE id = ?', - [$currentval, $thold_data['id']]); + if ($sample_row !== null) { + $sql[] = $sample_row; } } diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index 7692dbdf..bbc3e547 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -38,6 +38,7 @@ public static function setUpBeforeClass(): void { */ private function threshold(array $overrides = []) { return $overrides + [ + 'id' => 9, 'thold_id' => 9, 'local_data_id' => 4, 'name' => 'traffic_in', @@ -241,6 +242,28 @@ public function testNeverSampledThresholdLeavesTheTimestampPairUntouched(): void $this->assertStringNotContainsString('FROM_UNIXTIME', $call['sql']); $this->assertStringNotContainsString('oldvalue', $call['sql']); $this->assertSame(['', 9], $call['params']); + + CactiStubs::reset(); + $this->assertNull(thold_polling_sample_row($thold, ['traffic_in' => 'U'], '', 1300)); + $call = end(CactiStubs::$calls); + $this->assertStringNotContainsString('FROM_UNIXTIME', $call['sql']); + $this->assertSame(['', 9], $call['params']); + } + + /** + * @return void + */ + public function testPollerBuildsTheSamePersistedPair(): void { + $thold = $this->threshold(['lasttime' => 1000, 'oldvalue' => 100]); + + $this->assertSame( + "(9, 1, '2', FROM_UNIXTIME(1000), '100')", + thold_polling_sample_row($thold, [], 2, 1300) + ); + $this->assertSame( + "(9, 1, '2', FROM_UNIXTIME(1600), '700')", + thold_polling_sample_row($thold, ['traffic_in' => 700], 2, 1600) + ); } /** @@ -261,8 +284,6 @@ public function testDaemonAndPollerBothUseTheSharedPairPolicy(): void { $poller = file_get_contents(dirname(__DIR__, 2) . '/includes/polling.php'); $this->assertStringContainsString('thold_daemon_persist_sample($thold_data, $item, $currentval, $currenttime)', $daemon); - $this->assertStringContainsString('thold_sample_persistence($thold_data, $item, $currenttime)', $poller); - $this->assertStringContainsString("\$sample['lasttime']", $poller); - $this->assertStringContainsString("\$sample['oldvalue']", $poller); + $this->assertStringContainsString('thold_polling_sample_row($thold_data, $item, $currentval, $currenttime)', $poller); } } diff --git a/thold_functions.php b/thold_functions.php index 7ee1f4fe..32559c31 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -856,6 +856,32 @@ function thold_daemon_persist_sample(array $thold_data, array $item, $currentval [$currentval, $sample['lasttime'], $sample['oldvalue'], $thold_data['thold_id']]); } +/** + * Build one poller bulk-update row, or persist only status without a sample. + * + * @param array $thold_data + * @param array $item + * @param mixed $currentval + * @param int $currenttime + * + * @return string|null + */ +function thold_polling_sample_row(array $thold_data, array $item, $currentval, $currenttime) { + $sample = thold_sample_persistence($thold_data, $item, $currenttime); + + if ($sample['lasttime'] <= 0) { + db_execute_prepared('UPDATE thold_data + SET tcheck = 1, lastread = ? + WHERE id = ?', + [$currentval, $thold_data['id']]); + + return null; + } + + return '(' . (int) $thold_data['id'] . ', 1, ' . db_qstr($currentval) + . ', FROM_UNIXTIME(' . $sample['lasttime'] . '), ' . db_qstr($sample['oldvalue']) . ')'; +} + function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexed, &$item, &$currenttime) { // adjust the polling interval by the last read, if applicable $currenttime = $rrd_time_reindexed[$thold_data['local_data_id']]; From 603a8be199689b2e4a059b200cca5784f976eeeb Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 23:10:29 -0700 Subject: [PATCH 04/17] fix: bound counter gap recovery --- README.md | 3 +- includes/polling.php | 22 ++++- tests/Unit/TholdGetCurrentvalTest.php | 131 ++++++++++++++++++++------ thold_functions.php | 32 ++++--- 4 files changed, 143 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 7dbae9ee..d4b3890a 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,8 @@ exemptions, alert log retention, logging, etc. Counter thresholds preserve the previous value and timestamp together when a poll has no numeric sample, preventing the next rate from using mismatched -interval data. +interval data. A gap of more than two configured RRD steps starts a fresh +counter baseline instead of calculating a stale multi-day rate. 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 diff --git a/includes/polling.php b/includes/polling.php index 03a46bbc..d0d9fcbf 100644 --- a/includes/polling.php +++ b/includes/polling.php @@ -148,7 +148,8 @@ function thold_poller_output(&$rrd_update_array) { AND td.local_data_id IN($local_data_ids)"); if (cacti_sizeof($tholds)) { - $sql = []; + $sql = []; + $status_sql = []; foreach ($tholds as $thold_data) { thold_debug("Checking Threshold: Name: '" . $thold_data['thold_name'] . "', Graph: '" . $thold_data['local_graph_id'] . "'"); @@ -206,10 +207,12 @@ function thold_poller_output(&$rrd_update_array) { } } - $sample_row = thold_polling_sample_row($thold_data, $item, $currentval, $currenttime); + $sample_rows = thold_polling_sample_row($thold_data, $item, $currentval, $currenttime); - if ($sample_row !== null) { - $sql[] = $sample_row; + if ($sample_rows['sample_row'] !== null) { + $sql[] = $sample_rows['sample_row']; + } else { + $status_sql[] = $sample_rows['status_row']; } } @@ -234,6 +237,17 @@ function thold_poller_output(&$rrd_update_array) { set_config_option('time_last_change_thold', time()); } } + + if (cacti_sizeof($status_sql)) { + foreach (array_chunk($status_sql, 400) as $chunk) { + db_execute('INSERT INTO thold_data + (id, tcheck, lastread) + VALUES ' . implode(', ', $chunk) . ' + ON DUPLICATE KEY UPDATE + tcheck = VALUES(tcheck), + lastread = VALUES(lastread)'); + } + } } return $rrd_update_array; diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index bbc3e547..a30d38e9 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -45,7 +45,7 @@ private function threshold(array $overrides = []) { 'data_source_type_id' => self::COUNTER, 'rrd_step' => 300, 'rrd_maximum' => 0, - 'lasttime' => 0, + 'lasttime' => 1700000000, 'oldvalue' => 100, ]; } @@ -109,7 +109,7 @@ public function testCounterTreatsAPreviousReadingOfZeroAsReal(): void { * @return void */ public function testCounterWithNoPreviousReadingYieldsZero(): void { - $thold = $this->threshold(['oldvalue' => '']); + $thold = $this->threshold(['lasttime' => 0, 'oldvalue' => '']); $this->assertSame(0, $this->currentValue($thold, 600)); } @@ -121,7 +121,7 @@ public function testCounterWithNoPreviousReadingYieldsZero(): void { * @return void */ public function testThirtyTwoBitWrapUsesTheCorrectModulus(): void { - $thold = $this->threshold(['oldvalue' => 4294967290, 'rrd_step' => 1]); + $thold = $this->threshold(['oldvalue' => 4294967290, 'rrd_step' => 1, 'lasttime' => 1700000299]); $this->assertEqualsWithDelta(11, $this->currentValue($thold, 5), 1.0e-9); } @@ -130,7 +130,7 @@ public function testThirtyTwoBitWrapUsesTheCorrectModulus(): void { * @return void */ public function testSixtyFourBitWrapUsesTheCorrectModulus(): void { - $thold = $this->threshold(['oldvalue' => '18446744073709551610', 'rrd_step' => 1]); + $thold = $this->threshold(['oldvalue' => '18446744073709551610', 'rrd_step' => 1, 'lasttime' => 1700000299]); $this->assertEqualsWithDelta(11, $this->currentValue($thold, 5), 1.0e-9); } @@ -142,7 +142,7 @@ public function testSixtyFourBitWrapUsesTheCorrectModulus(): void { * @return void */ public function testSixtyFourBitWrapAcceptsScientificNotation(): void { - $thold = $this->threshold(['oldvalue' => '1.8446744073709552E+19', 'rrd_step' => 1]); + $thold = $this->threshold(['oldvalue' => '1.8446744073709552E+19', 'rrd_step' => 1, 'lasttime' => 1700000299]); $this->assertEqualsWithDelta(5, $this->currentValue($thold, 5), 1.0e-9); } @@ -210,6 +210,70 @@ public function testMissedSampleCarriesTheValueAndTimestampTogether(): void { ); } + /** + * @return array + */ + public static function emptyMaximumProvider() { + return [ + 'integer zero' => [0], + 'empty string' => [''], + 'explicit maximum' => [20000000], + ]; + } + + /** + * @dataProvider emptyMaximumProvider + * + * @param int|string $maximum + * + * @return void + */ + public function testMultiIntervalDeltaScalesTheResetGuard($maximum): void { + $thold = $this->threshold(['lasttime' => 1000, 'oldvalue' => 1000000000000, 'rrd_maximum' => $maximum]); + $reindexed = [4 => ['traffic_in' => 1007500000000]]; + $time_reindexed = [4 => 1600]; + $item = []; + $currenttime = 0; + + $this->assertEqualsWithDelta( + 12500000, + thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime), + 1.0e-9 + ); + } + + /** + * @return void + */ + public function testMultiIntervalWrapUsesTheWholeElapsedTime(): void { + $thold = $this->threshold(['lasttime' => 1000, 'oldvalue' => 4294967290, 'rrd_maximum' => 0]); + $reindexed = [4 => ['traffic_in' => 5]]; + $time_reindexed = [4 => 1600]; + $item = []; + $currenttime = 0; + + $this->assertEqualsWithDelta( + 11 / 600, + thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime), + 1.0e-9 + ); + } + + /** + * @return void + */ + public function testStaleCounterAndDeriveSamplesAreDiscarded(): void { + foreach ([self::COUNTER, self::DERIVE] as $type) { + $thold = $this->threshold(['data_source_type_id' => $type, 'lasttime' => 1000, 'oldvalue' => 100]); + $reindexed = [4 => ['traffic_in' => 700]]; + $time_reindexed = [4 => 1000 + 7 * 86400]; + $item = []; + $currenttime = 0; + + $this->assertSame(0, thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime)); + } + } + /** * @return void */ @@ -227,6 +291,28 @@ public function testDaemonPersistsThePairWithBoundParameters(): void { $this->assertSame([2, 1600, 700, 9], $call['params']); } + /** + * @return void + */ + public function testDaemonPropagatesPersistenceFailure(): void { + CactiStubs::willReturn('db_execute_prepared', false); + $this->assertFalse(thold_daemon_persist_sample( + $this->threshold(['lasttime' => 1000]), + [], + '', + 1300 + )); + + CactiStubs::reset(); + CactiStubs::willReturn('db_execute_prepared', false); + $this->assertFalse(thold_daemon_persist_sample( + $this->threshold(['lasttime' => 0]), + [], + '', + 1300 + )); + } + /** * @return void */ @@ -244,10 +330,11 @@ public function testNeverSampledThresholdLeavesTheTimestampPairUntouched(): void $this->assertSame(['', 9], $call['params']); CactiStubs::reset(); - $this->assertNull(thold_polling_sample_row($thold, ['traffic_in' => 'U'], '', 1300)); - $call = end(CactiStubs::$calls); - $this->assertStringNotContainsString('FROM_UNIXTIME', $call['sql']); - $this->assertSame(['', 9], $call['params']); + $this->assertSame( + ['sample_row' => null, 'status_row' => "(9, 1, '')"], + thold_polling_sample_row($thold, ['traffic_in' => 'U'], '', 1300) + ); + $this->assertSame([], CactiStubs::$calls); } /** @@ -256,14 +343,14 @@ public function testNeverSampledThresholdLeavesTheTimestampPairUntouched(): void public function testPollerBuildsTheSamePersistedPair(): void { $thold = $this->threshold(['lasttime' => 1000, 'oldvalue' => 100]); - $this->assertSame( - "(9, 1, '2', FROM_UNIXTIME(1000), '100')", - thold_polling_sample_row($thold, [], 2, 1300) - ); - $this->assertSame( - "(9, 1, '2', FROM_UNIXTIME(1600), '700')", - thold_polling_sample_row($thold, ['traffic_in' => 700], 2, 1600) - ); + $this->assertSame([ + 'sample_row' => "(9, 1, '2', FROM_UNIXTIME(1000), '100')", + 'status_row' => null, + ], thold_polling_sample_row($thold, [], 2, 1300)); + $this->assertSame([ + 'sample_row' => "(9, 1, '2', FROM_UNIXTIME(1600), '700')", + 'status_row' => null, + ], thold_polling_sample_row($thold, ['traffic_in' => 700], 2, 1600)); } /** @@ -276,14 +363,4 @@ public function testMissingPersistenceKeysFailClosed(): void { ); } - /** - * @return void - */ - public function testDaemonAndPollerBothUseTheSharedPairPolicy(): void { - $daemon = file_get_contents(dirname(__DIR__, 2) . '/thold_process.php'); - $poller = file_get_contents(dirname(__DIR__, 2) . '/includes/polling.php'); - - $this->assertStringContainsString('thold_daemon_persist_sample($thold_data, $item, $currentval, $currenttime)', $daemon); - $this->assertStringContainsString('thold_polling_sample_row($thold_data, $item, $currentval, $currenttime)', $poller); - } } diff --git a/thold_functions.php b/thold_functions.php index 32559c31..ef1e436f 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -857,29 +857,30 @@ function thold_daemon_persist_sample(array $thold_data, array $item, $currentval } /** - * Build one poller bulk-update row, or persist only status without a sample. + * Build one pure poller batching result for sample or status-only updates. * * @param array $thold_data * @param array $item * @param mixed $currentval * @param int $currenttime * - * @return string|null + * @return array{sample_row:string|null,status_row:string|null} */ function thold_polling_sample_row(array $thold_data, array $item, $currentval, $currenttime) { $sample = thold_sample_persistence($thold_data, $item, $currenttime); if ($sample['lasttime'] <= 0) { - db_execute_prepared('UPDATE thold_data - SET tcheck = 1, lastread = ? - WHERE id = ?', - [$currentval, $thold_data['id']]); - - return null; + return [ + 'sample_row' => null, + 'status_row' => '(' . (int) $thold_data['id'] . ', 1, ' . db_qstr($currentval) . ')', + ]; } - return '(' . (int) $thold_data['id'] . ', 1, ' . db_qstr($currentval) - . ', FROM_UNIXTIME(' . $sample['lasttime'] . '), ' . db_qstr($sample['oldvalue']) . ')'; + return [ + 'sample_row' => '(' . (int) $thold_data['id'] . ', 1, ' . db_qstr($currentval) + . ', FROM_UNIXTIME(' . $sample['lasttime'] . '), ' . db_qstr($sample['oldvalue']) . ')', + 'status_row' => null, + ]; } function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexed, &$item, &$currenttime) { @@ -900,6 +901,9 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe $step = read_config_option('poller_interval'); } + $rrd_step = max(1, (int) $thold_data['rrd_step']); + $previous_sample_usable = $thold_data['lasttime'] > 0 && $step > 0 && $step <= 2 * $rrd_step; + $currentval = ''; if (isset($rrd_reindexed[$thold_data['local_data_id']])) { @@ -909,7 +913,7 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe switch ($thold_data['data_source_type_id']) { case 2: // COUNTER // A previous reading of zero is a real reading, not a missing one. - if (is_numeric($thold_data['oldvalue']) && $thold_data['oldvalue'] !== '') { + if ($previous_sample_usable && is_numeric($thold_data['oldvalue']) && $thold_data['oldvalue'] !== '') { if ($item[$thold_data['name']] >= $thold_data['oldvalue']) { // Everything is Normal $currentval = $item[$thold_data['name']] - $thold_data['oldvalue']; @@ -946,7 +950,7 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe // assume counter reset if greater than max value if ($thold_data['rrd_maximum'] > 0 && ($currentval / $step) > $thold_data['rrd_maximum']) { $currentval = $item[$thold_data['name']] / $step; - } elseif ($thold_data['rrd_maximum'] == 0 && $currentval > 4.25E+9) { + } elseif ($thold_data['rrd_maximum'] == 0 && $currentval > 4.25E+9 * max(1, $step / $rrd_step)) { $currentval = $item[$thold_data['name']] / $step; } else { $currentval = $currentval / $step; @@ -957,7 +961,9 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe break; case 3: // DERIVE - $currentval = ($item[$thold_data['name']] - $thold_data['oldvalue']) / $step; + $currentval = $previous_sample_usable && is_numeric($thold_data['oldvalue']) + ? ($item[$thold_data['name']] - $thold_data['oldvalue']) / $step + : 0; break; case 4: // ABSOLUTE From a9716c7e31ac93905fa0b728a0818d537f308bc3 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 23:11:08 -0700 Subject: [PATCH 05/17] style: format counter recovery tests --- tests/Unit/TholdGetCurrentvalTest.php | 5 ++--- thold_functions.php | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index a30d38e9..b5b0572d 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -215,8 +215,8 @@ public function testMissedSampleCarriesTheValueAndTimestampTogether(): void { */ public static function emptyMaximumProvider() { return [ - 'integer zero' => [0], - 'empty string' => [''], + 'integer zero' => [0], + 'empty string' => [''], 'explicit maximum' => [20000000], ]; } @@ -362,5 +362,4 @@ public function testMissingPersistenceKeysFailClosed(): void { thold_sample_persistence([], ['traffic_in' => 700], 1600) ); } - } diff --git a/thold_functions.php b/thold_functions.php index ef1e436f..afd1f035 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -901,7 +901,7 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe $step = read_config_option('poller_interval'); } - $rrd_step = max(1, (int) $thold_data['rrd_step']); + $rrd_step = max(1, (int) $thold_data['rrd_step']); $previous_sample_usable = $thold_data['lasttime'] > 0 && $step > 0 && $step <= 2 * $rrd_step; $currentval = ''; From 14da7ce7f40a9997f08fda09dda563b1bce939f2 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 23:22:13 -0700 Subject: [PATCH 06/17] fix: treat unusable counter baselines as unknown --- README.md | 9 ++-- tests/Unit/TholdGetCurrentvalTest.php | 72 ++++++++++++++++++++++----- thold_functions.php | 32 +++++++++--- 3 files changed, 89 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index d4b3890a..583f1f23 100644 --- a/README.md +++ b/README.md @@ -27,10 +27,11 @@ 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. -Counter thresholds preserve the previous value and timestamp together when a -poll has no numeric sample, preventing the next rate from using mismatched -interval data. A gap of more than two configured RRD steps starts a fresh -counter baseline instead of calculating a stale multi-day rate. +Counter and derive thresholds preserve the previous value and timestamp +together when a poll has no numeric sample, preventing the next rate from using +mismatched interval data. A gap of more than two effective sampling intervals +(the greater of the RRD step and poller interval) is treated as unknown while +the current sample starts a fresh baseline instead of producing a stale rate. 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 diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index b5b0572d..caeccbaa 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -211,13 +211,13 @@ public function testMissedSampleCarriesTheValueAndTimestampTogether(): void { } /** - * @return array + * @return array */ public static function emptyMaximumProvider() { return [ - 'integer zero' => [0], - 'empty string' => [''], - 'explicit maximum' => [20000000], + 'integer zero' => [0, 1007500000000], + 'empty string' => ['', 1008500000100], + 'explicit maximum' => [20000000, 1007500000000], ]; } @@ -225,21 +225,19 @@ public static function emptyMaximumProvider() { * @dataProvider emptyMaximumProvider * * @param int|string $maximum + * @param int $reading * * @return void */ - public function testMultiIntervalDeltaScalesTheResetGuard($maximum): void { + public function testMultiIntervalDeltaScalesTheResetGuard($maximum, $reading): void { $thold = $this->threshold(['lasttime' => 1000, 'oldvalue' => 1000000000000, 'rrd_maximum' => $maximum]); - $reindexed = [4 => ['traffic_in' => 1007500000000]]; + $reindexed = [4 => ['traffic_in' => $reading]]; $time_reindexed = [4 => 1600]; $item = []; $currenttime = 0; - $this->assertEqualsWithDelta( - 12500000, - thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime), - 1.0e-9 - ); + $expected = $maximum === '' ? $reading / 600 : 12500000; + $this->assertEqualsWithDelta($expected, thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime), 1.0e-9); } /** @@ -270,7 +268,57 @@ public function testStaleCounterAndDeriveSamplesAreDiscarded(): void { $item = []; $currenttime = 0; - $this->assertSame(0, thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime)); + $this->assertSame('', thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime)); + } + } + + /** + * @return array + */ + public static function invalidRrdStepProvider() { + return [ + 'zero' => [0], + 'null' => [null], + 'non-numeric' => ['invalid'], + ]; + } + + /** + * @dataProvider invalidRrdStepProvider + * + * @param mixed $rrd_step + * + * @return void + */ + public function testInvalidRrdStepFallsBackToThePollerInterval($rrd_step): void { + CactiStubs::$configOptions['poller_interval'] = 300; + $thold = $this->threshold(['rrd_step' => $rrd_step]); + + $this->assertEqualsWithDelta(2, $this->currentValue($thold, 700), 1.0e-9); + } + + /** + * @return void + */ + public function testEvaluationCadenceMayExceedTheRrdStep(): void { + CactiStubs::$configOptions['poller_interval'] = 300; + $thold = $this->threshold(['rrd_step' => 60]); + + $this->assertEqualsWithDelta(2, $this->currentValue($thold, 700), 1.0e-9); + } + + /** + * @return void + */ + public function testOutOfOrderCounterAndDeriveSamplesAreUnknown(): void { + foreach ([self::COUNTER, self::DERIVE] as $type) { + $thold = $this->threshold(['data_source_type_id' => $type, 'lasttime' => 1700000400]); + $reindexed = [4 => ['traffic_in' => 700]]; + $time_reindexed = [4 => 1700000300]; + $item = []; + $currenttime = 0; + + $this->assertSame('', thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime)); } } diff --git a/thold_functions.php b/thold_functions.php index afd1f035..2f555b42 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -901,8 +901,20 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe $step = read_config_option('poller_interval'); } - $rrd_step = max(1, (int) $thold_data['rrd_step']); - $previous_sample_usable = $thold_data['lasttime'] > 0 && $step > 0 && $step <= 2 * $rrd_step; + $poller_interval = read_config_option('poller_interval'); + + if (!is_numeric($poller_interval) || $poller_interval <= 0) { + $poller_interval = 300; + } + + $rrd_step = $thold_data['rrd_step']; + + if (!is_numeric($rrd_step) || $rrd_step <= 0) { + $rrd_step = $poller_interval; + } + + $sample_interval = max((float) $rrd_step, (float) $poller_interval); + $previous_sample_usable = $thold_data['lasttime'] > 0 && $step > 0 && $step <= 2 * $sample_interval; $currentval = ''; @@ -947,23 +959,27 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe } } + $rrd_maximum = is_numeric($thold_data['rrd_maximum']) ? (float) $thold_data['rrd_maximum'] : 0.0; + // assume counter reset if greater than max value - if ($thold_data['rrd_maximum'] > 0 && ($currentval / $step) > $thold_data['rrd_maximum']) { + if ($rrd_maximum > 0 && ($currentval / $step) > $rrd_maximum) { $currentval = $item[$thold_data['name']] / $step; - } elseif ($thold_data['rrd_maximum'] == 0 && $currentval > 4.25E+9 * max(1, $step / $rrd_step)) { + } elseif ($rrd_maximum === 0.0 && $currentval > 4.25E+9 * max(1, $step / $rrd_step)) { $currentval = $item[$thold_data['name']] / $step; } else { $currentval = $currentval / $step; } } else { - $currentval = 0; + $currentval = $thold_data['lasttime'] > 0 ? '' : 0; } break; case 3: // DERIVE - $currentval = $previous_sample_usable && is_numeric($thold_data['oldvalue']) - ? ($item[$thold_data['name']] - $thold_data['oldvalue']) / $step - : 0; + if ($previous_sample_usable && is_numeric($thold_data['oldvalue'])) { + $currentval = ($item[$thold_data['name']] - $thold_data['oldvalue']) / $step; + } else { + $currentval = $thold_data['lasttime'] > 0 ? '' : 0; + } break; case 4: // ABSOLUTE From 00f8ada0522a100e5b086f10a6824dc5ce6a1a29 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 23:39:14 -0700 Subject: [PATCH 07/17] fix: preserve unknown counter maximum semantics --- includes/polling.php | 10 ++--- tests/Unit/TholdGetCurrentvalTest.php | 38 ++++++++++++++++--- tests/bootstrap-unit.php | 8 ++++ thold_functions.php | 54 +++++++++++++++++++++------ 4 files changed, 85 insertions(+), 25 deletions(-) diff --git a/includes/polling.php b/includes/polling.php index d0d9fcbf..14515cfa 100644 --- a/includes/polling.php +++ b/includes/polling.php @@ -211,7 +211,7 @@ function thold_poller_output(&$rrd_update_array) { if ($sample_rows['sample_row'] !== null) { $sql[] = $sample_rows['sample_row']; - } else { + } elseif ($sample_rows['status_row'] !== null) { $status_sql[] = $sample_rows['status_row']; } } @@ -230,12 +230,6 @@ function thold_poller_output(&$rrd_update_array) { oldvalue = VALUES(oldvalue)'); } - // accommodate deleted tholds - db_execute('DELETE FROM thold_data WHERE local_data_id = 0'); - - if (db_affected_rows() > 0) { - set_config_option('time_last_change_thold', time()); - } } if (cacti_sizeof($status_sql)) { @@ -248,6 +242,8 @@ function thold_poller_output(&$rrd_update_array) { lastread = VALUES(lastread)'); } } + + thold_polling_cleanup(cacti_sizeof($sql) || cacti_sizeof($status_sql)); } return $rrd_update_array; diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index caeccbaa..2de7f256 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -211,13 +211,15 @@ public function testMissedSampleCarriesTheValueAndTimestampTogether(): void { } /** - * @return array + * @return array */ public static function emptyMaximumProvider() { return [ - 'integer zero' => [0, 1007500000000], - 'empty string' => ['', 1008500000100], - 'explicit maximum' => [20000000, 1007500000000], + 'integer zero' => [0, 1007500000000, 12500000], + 'empty string' => ['', 1008500000100, 8500000100 / 600], + 'unknown maximum' => ['U', 1008500000100, 8500000100 / 600], + 'unresolved if speed' => ['|query_ifSpeed|', 1008500000100, 8500000100 / 600], + 'explicit maximum' => [20000000, 1007500000000, 12500000], ]; } @@ -226,17 +228,22 @@ public static function emptyMaximumProvider() { * * @param int|string $maximum * @param int $reading + * @param float $expected * * @return void */ - public function testMultiIntervalDeltaScalesTheResetGuard($maximum, $reading): void { + public function testMultiIntervalDeltaScalesTheResetGuard($maximum, $reading, $expected): void { + if ($maximum === '|query_ifSpeed|') { + CactiStubs::willReturn('db_fetch_row_prepared', ['host_id' => 1, 'snmp_query_id' => 2, 'snmp_index' => 'eth0']); + CactiStubs::willReturn('db_fetch_cell_prepared', ''); + } + $thold = $this->threshold(['lasttime' => 1000, 'oldvalue' => 1000000000000, 'rrd_maximum' => $maximum]); $reindexed = [4 => ['traffic_in' => $reading]]; $time_reindexed = [4 => 1600]; $item = []; $currenttime = 0; - $expected = $maximum === '' ? $reading / 600 : 12500000; $this->assertEqualsWithDelta($expected, thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime), 1.0e-9); } @@ -409,5 +416,24 @@ public function testMissingPersistenceKeysFailClosed(): void { ['lasttime' => 0, 'oldvalue' => null], thold_sample_persistence([], ['traffic_in' => 700], 1600) ); + $this->assertFalse(thold_daemon_persist_sample([], ['traffic_in' => 700], 2, 1600)); + $this->assertSame( + ['sample_row' => null, 'status_row' => null], + thold_polling_sample_row([], ['traffic_in' => 700], 2, 1600) + ); + } + + /** + * @return void + */ + public function testPollerCleanupRunsForEitherBatchType(): void { + thold_polling_cleanup(false); + $this->assertSame([], CactiStubs::$calls); + + CactiStubs::willReturn('db_affected_rows', 1); + thold_polling_cleanup(true); + $this->assertSame('db_execute', CactiStubs::$calls[0]['fn']); + $this->assertStringContainsString('local_data_id = 0', CactiStubs::$calls[0]['sql']); + $this->assertArrayHasKey('time_last_change_thold', CactiStubs::$configOptions); } } diff --git a/tests/bootstrap-unit.php b/tests/bootstrap-unit.php index 6c4a8e4f..cdb9f015 100644 --- a/tests/bootstrap-unit.php +++ b/tests/bootstrap-unit.php @@ -494,6 +494,14 @@ function rrdtool_function_interface_speed($data_local) { } } +if (!function_exists('substitute_snmp_query_data')) { + function substitute_snmp_query_data($value, $host_id, $snmp_query_id, $snmp_index) { + CactiStubs::record('substitute_snmp_query_data', (string) $value, [$host_id, $snmp_query_id, $snmp_index]); + + return CactiStubs::nextReturn('substitute_snmp_query_data', $value); + } +} + if (!function_exists('get_timeinstate')) { function get_timeinstate($host) { return CactiStubs::nextReturn('get_timeinstate', '1 day'); diff --git a/thold_functions.php b/thold_functions.php index 2f555b42..aae3793a 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -840,20 +840,26 @@ function thold_sample_persistence(array $thold_data, array $item, $currenttime) * @return bool */ function thold_daemon_persist_sample(array $thold_data, array $item, $currentval, $currenttime) { + $id = (int) ($thold_data['thold_id'] ?? 0); + + if ($id <= 0) { + return false; + } + $sample = thold_sample_persistence($thold_data, $item, $currenttime); if ($sample['lasttime'] <= 0) { return db_execute_prepared('UPDATE thold_data SET tcheck = 1, lastread = ? WHERE id = ?', - [$currentval, $thold_data['thold_id']]); + [$currentval, $id]); } return db_execute_prepared('UPDATE thold_data SET tcheck = 1, lastread = ?, lasttime = FROM_UNIXTIME(?), oldvalue = ? WHERE id = ?', - [$currentval, $sample['lasttime'], $sample['oldvalue'], $thold_data['thold_id']]); + [$currentval, $sample['lasttime'], $sample['oldvalue'], $id]); } /** @@ -867,25 +873,55 @@ function thold_daemon_persist_sample(array $thold_data, array $item, $currentval * @return array{sample_row:string|null,status_row:string|null} */ function thold_polling_sample_row(array $thold_data, array $item, $currentval, $currenttime) { + $id = (int) ($thold_data['id'] ?? 0); + + if ($id <= 0) { + return ['sample_row' => null, 'status_row' => null]; + } + $sample = thold_sample_persistence($thold_data, $item, $currenttime); if ($sample['lasttime'] <= 0) { return [ 'sample_row' => null, - 'status_row' => '(' . (int) $thold_data['id'] . ', 1, ' . db_qstr($currentval) . ')', + 'status_row' => '(' . $id . ', 1, ' . db_qstr($currentval) . ')', ]; } return [ - 'sample_row' => '(' . (int) $thold_data['id'] . ', 1, ' . db_qstr($currentval) + 'sample_row' => '(' . $id . ', 1, ' . db_qstr($currentval) . ', FROM_UNIXTIME(' . $sample['lasttime'] . '), ' . db_qstr($sample['oldvalue']) . ')', 'status_row' => null, ]; } +/** + * Remove rows deleted during polling after either update batch ran. + * + * @param bool $has_updates + * + * @return void + */ +function thold_polling_cleanup($has_updates) { + if (!$has_updates) { + return; + } + + db_execute('DELETE FROM thold_data WHERE local_data_id = 0'); + + if (db_affected_rows() > 0) { + set_config_option('time_last_change_thold', time()); + } +} + function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexed, &$item, &$currenttime) { // adjust the polling interval by the last read, if applicable $currenttime = $rrd_time_reindexed[$thold_data['local_data_id']]; + $poller_interval = read_config_option('poller_interval'); + + if (!is_numeric($poller_interval) || $poller_interval <= 0) { + $poller_interval = 300; + } if ($thold_data['lasttime'] > 0) { if (is_numeric($currenttime)) { @@ -898,13 +934,7 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe } if (empty($step)) { - $step = read_config_option('poller_interval'); - } - - $poller_interval = read_config_option('poller_interval'); - - if (!is_numeric($poller_interval) || $poller_interval <= 0) { - $poller_interval = 300; + $step = $poller_interval; } $rrd_step = $thold_data['rrd_step']; @@ -959,7 +989,7 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe } } - $rrd_maximum = is_numeric($thold_data['rrd_maximum']) ? (float) $thold_data['rrd_maximum'] : 0.0; + $rrd_maximum = is_numeric($thold_data['rrd_maximum']) ? (float) $thold_data['rrd_maximum'] : null; // assume counter reset if greater than max value if ($rrd_maximum > 0 && ($currentval / $step) > $rrd_maximum) { From 97cdbc48c38b9163b80b2c684a7c4571d45fae7d Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 23:39:53 -0700 Subject: [PATCH 08/17] test: cover validated poller interval fallback --- tests/Unit/TholdGetCurrentvalTest.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index 2de7f256..645561d7 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -314,6 +314,20 @@ public function testEvaluationCadenceMayExceedTheRrdStep(): void { $this->assertEqualsWithDelta(2, $this->currentValue($thold, 700), 1.0e-9); } + /** + * @return void + */ + public function testFirstAbsoluteSampleUsesTheValidatedPollerInterval(): void { + CactiStubs::$configOptions['poller_interval'] = 300; + $thold = $this->threshold([ + 'data_source_type_id' => self::ABSOLUTE, + 'lasttime' => 0, + 'rrd_step' => 0, + ]); + + $this->assertEqualsWithDelta(2, $this->currentValue($thold, 600), 1.0e-9); + } + /** * @return void */ From af6b1ffa7b053f58c36be7a8813594e5645a4e4c Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 23:48:57 -0700 Subject: [PATCH 09/17] fix: retain trustworthy counter baselines --- tests/Unit/TholdGetCurrentvalTest.php | 56 +++++++++++++++++++++++---- thold_functions.php | 15 ++++--- 2 files changed, 59 insertions(+), 12 deletions(-) diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index 645561d7..3bdfb8fb 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -211,22 +211,23 @@ public function testMissedSampleCarriesTheValueAndTimestampTogether(): void { } /** - * @return array + * @return array */ public static function emptyMaximumProvider() { return [ - 'integer zero' => [0, 1007500000000, 12500000], - 'empty string' => ['', 1008500000100, 8500000100 / 600], - 'unknown maximum' => ['U', 1008500000100, 8500000100 / 600], - 'unresolved if speed' => ['|query_ifSpeed|', 1008500000100, 8500000100 / 600], - 'explicit maximum' => [20000000, 1007500000000, 12500000], + 'integer zero' => [0, 1009000000100, 1009000000100 / 600], + 'empty string' => ['', 1009000000100, 1009000000100 / 600], + 'null maximum' => [null, 1009000000100, 1009000000100 / 600], + 'unknown maximum' => ['U', 1009000000100, 9000000100 / 600], + 'unresolved if speed' => ['|query_ifSpeed|', 1009000000100, 9000000100 / 600], + 'explicit maximum' => [20000000, 1015000000000, 1015000000000 / 600], ]; } /** * @dataProvider emptyMaximumProvider * - * @param int|string $maximum + * @param int|string|null $maximum * @param int $reading * @param float $expected * @@ -264,6 +265,21 @@ public function testMultiIntervalWrapUsesTheWholeElapsedTime(): void { ); } + /** + * @return void + */ + public function testWrapResetGuardUsesTheEffectiveSampleInterval(): void { + CactiStubs::$configOptions['poller_interval'] = 300; + $thold = $this->threshold([ + 'lasttime' => 1700000000, + 'oldvalue' => 1000, + 'rrd_step' => 60, + 'rrd_maximum' => 0, + ]); + + $this->assertEqualsWithDelta(999 / 300, $this->currentValue($thold, 999), 1.0e-9); + } + /** * @return void */ @@ -341,6 +357,32 @@ public function testOutOfOrderCounterAndDeriveSamplesAreUnknown(): void { $this->assertSame('', thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime)); } + + $this->assertSame( + ['lasttime' => 1700000400, 'oldvalue' => 100], + thold_sample_persistence($thold, ['traffic_in' => 700], 1700000300) + ); + $this->assertSame([ + 'sample_row' => "(9, 1, '', FROM_UNIXTIME(1700000400), '100')", + 'status_row' => null, + ], thold_polling_sample_row($thold, ['traffic_in' => 700], '', 1700000300)); + + CactiStubs::reset(); + $this->assertTrue(thold_daemon_persist_sample($thold, ['traffic_in' => 700], '', 1700000300)); + $call = end(CactiStubs::$calls); + $this->assertSame(['', 1700000400, 100, 9], $call['params']); + } + + /** + * @return void + */ + public function testDeriveWithAnInvalidPriorValueIsUnknown(): void { + $thold = $this->threshold([ + 'data_source_type_id' => self::DERIVE, + 'oldvalue' => 'U', + ]); + + $this->assertSame('', $this->currentValue($thold, 700)); } /** diff --git a/thold_functions.php b/thold_functions.php index aae3793a..9d8519b5 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -819,12 +819,14 @@ function thold_sample_persistence(array $thold_data, array $item, $currenttime) $name = (string) ($thold_data['name'] ?? ''); $currenttime = (int) $currenttime; - if ($name !== '' && $currenttime > 0 && isset($item[$name]) && is_numeric($item[$name])) { + $lasttime = (int) ($thold_data['lasttime'] ?? 0); + + if ($name !== '' && $currenttime > 0 && $currenttime >= $lasttime && isset($item[$name]) && is_numeric($item[$name])) { return ['lasttime' => $currenttime, 'oldvalue' => $item[$name]]; } return [ - 'lasttime' => (int) ($thold_data['lasttime'] ?? 0), + 'lasttime' => $lasttime, 'oldvalue' => $thold_data['oldvalue'] ?? null, ]; } @@ -964,7 +966,7 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe $currentval = thold_counter_wrap_delta($thold_data['oldvalue'], $item[$thold_data['name']]); } - if (strpos($thold_data['rrd_maximum'], '|query_') !== false) { + if (strpos((string) $thold_data['rrd_maximum'], '|query_') !== false) { $data_local = db_fetch_row_prepared('SELECT * FROM data_local WHERE id = ?', @@ -989,12 +991,15 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe } } - $rrd_maximum = is_numeric($thold_data['rrd_maximum']) ? (float) $thold_data['rrd_maximum'] : null; + $maximum_value = $thold_data['rrd_maximum'] ?? ''; + $rrd_maximum = trim((string) $maximum_value) === '' + ? 0.0 + : (is_numeric($maximum_value) ? (float) $maximum_value : null); // assume counter reset if greater than max value if ($rrd_maximum > 0 && ($currentval / $step) > $rrd_maximum) { $currentval = $item[$thold_data['name']] / $step; - } elseif ($rrd_maximum === 0.0 && $currentval > 4.25E+9 * max(1, $step / $rrd_step)) { + } elseif ($rrd_maximum === 0.0 && $currentval > 4.25E+9 * max(1, $step / $sample_interval)) { $currentval = $item[$thold_data['name']] / $step; } else { $currentval = $currentval / $step; From 4b048f23c8b21a99f68ff27d48bacd604858462f Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 18 Aug 2026 00:01:22 -0700 Subject: [PATCH 10/17] fix: propagate unknown rate samples safely --- README.md | 11 +++---- tests/Unit/TholdGetCurrentvalTest.php | 43 +++++++++++++++++++++++++++ thold_functions.php | 19 +++++++++--- 3 files changed, 64 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 583f1f23..357030b6 100644 --- a/README.md +++ b/README.md @@ -27,11 +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. -Counter and derive thresholds preserve the previous value and timestamp -together when a poll has no numeric sample, preventing the next rate from using -mismatched interval data. A gap of more than two effective sampling intervals -(the greater of the RRD step and poller interval) is treated as unknown while -the current sample starts a fresh baseline instead of producing a stale rate. +Counter, derive, and absolute rate thresholds preserve the previous value and +timestamp together when a poll has no numeric sample, preventing the next rate +from using mismatched interval data. A gap of more than two effective sampling +intervals (the greater of the RRD step and poller interval) is treated as +unknown while the current sample starts a fresh baseline instead of producing +a stale rate. Gauge readings remain valid across such a gap. 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 diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index 3bdfb8fb..7876ce41 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -83,6 +83,34 @@ public function testAbsoluteDividesTheReadingByTheStep(): void { $this->assertEqualsWithDelta(2, $this->currentValue($thold, 600), 1.0e-9); } + /** + * @return void + */ + public function testAbsoluteRejectsStaleAndOutOfOrderIntervals(): void { + foreach ([1000 + 86400, 900] as $sample_time) { + $thold = $this->threshold(['data_source_type_id' => self::ABSOLUTE, 'lasttime' => 1000]); + $reindexed = [4 => ['traffic_in' => 600]]; + $time_reindexed = [4 => $sample_time]; + $item = []; + $currenttime = 0; + + $this->assertSame('', thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime)); + } + } + + /** + * @return void + */ + public function testGaugeRemainsValidAcrossAGap(): void { + $thold = $this->threshold(['data_source_type_id' => self::GAUGE, 'lasttime' => 1000]); + $reindexed = [4 => ['traffic_in' => 42]]; + $time_reindexed = [4 => 1000 + 7 * 86400]; + $item = []; + $currenttime = 0; + + $this->assertSame(42, thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime)); + } + /** * @return void */ @@ -176,6 +204,17 @@ public function testMissingDataSourceYieldsTheNoValueSentinel(): void { $this->assertSame('', thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime)); } + /** + * @return void + */ + public function testLowerUpperCombinationRejectsUnknownInputs(): void { + $thold = ['local_data_id' => 4, 'upper_ds' => 'upper']; + + $this->assertSame('', thold_calculate_lower_upper($thold, '', [4 => ['upper' => 5]])); + $this->assertSame('', thold_calculate_lower_upper($thold, 7, [4 => ['upper' => 'U']])); + $this->assertSame((5 << 32) + 7, thold_calculate_lower_upper($thold, 7, [4 => ['upper' => 5]])); + } + /** * @return void */ @@ -362,6 +401,10 @@ public function testOutOfOrderCounterAndDeriveSamplesAreUnknown(): void { ['lasttime' => 1700000400, 'oldvalue' => 100], thold_sample_persistence($thold, ['traffic_in' => 700], 1700000300) ); + $this->assertSame( + ['lasttime' => 1700000400, 'oldvalue' => 100], + thold_sample_persistence($thold, ['traffic_in' => 700], 1700000400) + ); $this->assertSame([ 'sample_row' => "(9, 1, '', FROM_UNIXTIME(1700000400), '100')", 'status_row' => null, diff --git a/thold_functions.php b/thold_functions.php index 9d8519b5..e3b65166 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -821,7 +821,7 @@ function thold_sample_persistence(array $thold_data, array $item, $currenttime) $lasttime = (int) ($thold_data['lasttime'] ?? 0); - if ($name !== '' && $currenttime > 0 && $currenttime >= $lasttime && isset($item[$name]) && is_numeric($item[$name])) { + if ($name !== '' && $currenttime > $lasttime && isset($item[$name]) && is_numeric($item[$name])) { return ['lasttime' => $currenttime, 'oldvalue' => $item[$name]]; } @@ -1018,7 +1018,9 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe break; case 4: // ABSOLUTE - $currentval = $item[$thold_data['name']] / $step; + $currentval = ($thold_data['lasttime'] <= 0 || $previous_sample_usable) + ? $item[$thold_data['name']] / $step + : ''; break; case 1: // GAUGE @@ -1384,9 +1386,18 @@ function thold_calculate_percent($thold, $currentval, $rrd_reindexed) { function thold_calculate_lower_upper($thold, $currentval, $rrd_reindexed) { $ds = $thold['upper_ds']; + if (!is_numeric($currentval)) { + return ''; + } + if (isset($rrd_reindexed[$thold['local_data_id']][$ds])) { - $t = $rrd_reindexed[$thold['local_data_id']][$thold['upper_ds']]; - $currentval = ($t << 32) + $currentval; + $t = $rrd_reindexed[$thold['local_data_id']][$thold['upper_ds']]; + + if (!is_numeric($t)) { + return ''; + } + + $currentval = ((int) $t << 32) + $currentval; } return $currentval; From f2a461bf7e5557222308018efb39637221e5a603 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 18 Aug 2026 00:17:47 -0700 Subject: [PATCH 11/17] fix: validate effective counter step --- tests/Unit/TholdGetCurrentvalTest.php | 14 +++++++++++--- thold_functions.php | 22 ++++++++++++---------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index 7876ce41..9fafbc00 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -354,9 +354,13 @@ public static function invalidRrdStepProvider() { */ public function testInvalidRrdStepFallsBackToThePollerInterval($rrd_step): void { CactiStubs::$configOptions['poller_interval'] = 300; - $thold = $this->threshold(['rrd_step' => $rrd_step]); + $thold = $this->threshold([ + 'data_source_type_id' => self::ABSOLUTE, + 'lasttime' => 0, + 'rrd_step' => $rrd_step, + ]); - $this->assertEqualsWithDelta(2, $this->currentValue($thold, 700), 1.0e-9); + $this->assertEqualsWithDelta(2, $this->currentValue($thold, 600), 1.0e-9); } /** @@ -505,6 +509,10 @@ public function testPollerBuildsTheSamePersistedPair(): void { 'sample_row' => "(9, 1, '2', FROM_UNIXTIME(1600), '700')", 'status_row' => null, ], thold_polling_sample_row($thold, ['traffic_in' => 700], 2, 1600)); + $this->assertSame([ + 'sample_row' => "(9, 1, '2', FROM_UNIXTIME(1000), '')", + 'status_row' => null, + ], thold_polling_sample_row($this->threshold(['lasttime' => 1000, 'oldvalue' => null]), [], 2, 1300)); } /** @@ -531,7 +539,7 @@ public function testPollerCleanupRunsForEitherBatchType(): void { CactiStubs::willReturn('db_affected_rows', 1); thold_polling_cleanup(true); - $this->assertSame('db_execute', CactiStubs::$calls[0]['fn']); + $this->assertSame('db_execute_prepared', CactiStubs::$calls[0]['fn']); $this->assertStringContainsString('local_data_id = 0', CactiStubs::$calls[0]['sql']); $this->assertArrayHasKey('time_last_change_thold', CactiStubs::$configOptions); } diff --git a/thold_functions.php b/thold_functions.php index e3b65166..0c45e8f8 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -909,7 +909,7 @@ function thold_polling_cleanup($has_updates) { return; } - db_execute('DELETE FROM thold_data WHERE local_data_id = 0'); + db_execute_prepared('DELETE FROM thold_data WHERE local_data_id = 0'); if (db_affected_rows() > 0) { set_config_option('time_last_change_thold', time()); @@ -935,18 +935,20 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe $step = $thold_data['rrd_step']; } - if (empty($step)) { - $step = $poller_interval; - } + $elapsed_step = $step; - $rrd_step = $thold_data['rrd_step']; - - if (!is_numeric($rrd_step) || $rrd_step <= 0) { - $rrd_step = $poller_interval; + if (!is_numeric($step) || $step <= 0) { + $step = $poller_interval; } - $sample_interval = max((float) $rrd_step, (float) $poller_interval); - $previous_sample_usable = $thold_data['lasttime'] > 0 && $step > 0 && $step <= 2 * $sample_interval; + $rrd_step = is_numeric($thold_data['rrd_step']) && $thold_data['rrd_step'] > 0 + ? (float) $thold_data['rrd_step'] + : 0.0; + $sample_interval = max($rrd_step, (float) $poller_interval); + $previous_sample_usable = $thold_data['lasttime'] > 0 + && is_numeric($elapsed_step) + && $elapsed_step > 0 + && $elapsed_step <= 2 * $sample_interval; $currentval = ''; From a69a7ee711e4416ce2c6a82bcc9c226f37b95307 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 18 Aug 2026 00:30:42 -0700 Subject: [PATCH 12/17] fix: preserve alerts across unknown rate gaps --- README.md | 8 +++-- includes/polling.php | 2 +- tests/Unit/TholdGetCurrentvalTest.php | 51 ++++++++++++++++++++++----- thold_functions.php | 27 ++++++++------ thold_process.php | 8 +++-- 5 files changed, 70 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 357030b6..9cb3ac66 100644 --- a/README.md +++ b/README.md @@ -30,9 +30,11 @@ exemptions, alert log retention, logging, etc. Counter, derive, and absolute rate thresholds preserve the previous value and timestamp together when a poll has no numeric sample, preventing the next rate from using mismatched interval data. A gap of more than two effective sampling -intervals (the greater of the RRD step and poller interval) is treated as -unknown while the current sample starts a fresh baseline instead of producing -a stale rate. Gauge readings remain valid across such a gap. +intervals (the greater of the RRD step and poller interval), or the configured +RRD heartbeat when available, is treated as unknown while the current sample +starts a fresh baseline instead of producing a stale rate. Unknown rates are +not evaluated, so existing alert state is preserved. Gauge readings remain +valid across such a gap. 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 diff --git a/includes/polling.php b/includes/polling.php index 14515cfa..c7a82e26 100644 --- a/includes/polling.php +++ b/includes/polling.php @@ -138,7 +138,7 @@ function thold_poller_output(&$rrd_update_array) { td.cdef, td.local_data_id, td.data_template_rrd_id, td.lastread, UNIX_TIMESTAMP(td.lasttime) AS lasttime, td.oldvalue, td.data_source_name AS name, dtr.data_source_type_id, - dtd.rrd_step, dtr.rrd_maximum + dtd.rrd_step, dtr.rrd_maximum, dtr.rrd_heartbeat FROM thold_data AS td LEFT JOIN data_template_rrd AS dtr ON dtr.id = td.data_template_rrd_id diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index 9fafbc00..a463f2b1 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -136,10 +136,10 @@ public function testCounterTreatsAPreviousReadingOfZeroAsReal(): void { /** * @return void */ - public function testCounterWithNoPreviousReadingYieldsZero(): void { + public function testCounterWithNoPreviousReadingYieldsUnknown(): void { $thold = $this->threshold(['lasttime' => 0, 'oldvalue' => '']); - $this->assertSame(0, $this->currentValue($thold, 600)); + $this->assertSame('', $this->currentValue($thold, 600)); } /** @@ -373,6 +373,41 @@ public function testEvaluationCadenceMayExceedTheRrdStep(): void { $this->assertEqualsWithDelta(2, $this->currentValue($thold, 700), 1.0e-9); } + /** + * @return void + */ + public function testRrdHeartbeatControlsGapAcceptance(): void { + $accepted = $this->threshold(['lasttime' => 1000, 'rrd_heartbeat' => 1800]); + $reindexed = [4 => ['traffic_in' => 700]]; + $time_reindexed = [4 => 1900]; + $item = []; + $currenttime = 0; + + $this->assertEqualsWithDelta( + 600 / 900, + thold_get_currentval($accepted, $reindexed, $time_reindexed, $item, $currenttime), + 1.0e-9 + ); + + $rejected = $accepted; + $rejected['rrd_heartbeat'] = 300; + $time_reindexed[4] = 1400; + $this->assertSame('', thold_get_currentval($rejected, $reindexed, $time_reindexed, $item, $currenttime)); + } + + /** + * @return void + */ + public function testNonNumericSampleTimeUsesTheRrdStep(): void { + $thold = $this->threshold(); + $reindexed = [4 => ['traffic_in' => 700]]; + $time_reindexed = [4 => 'U']; + $item = []; + $currenttime = 0; + + $this->assertEqualsWithDelta(2, thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime), 1.0e-9); + } + /** * @return void */ @@ -410,14 +445,14 @@ public function testOutOfOrderCounterAndDeriveSamplesAreUnknown(): void { thold_sample_persistence($thold, ['traffic_in' => 700], 1700000400) ); $this->assertSame([ - 'sample_row' => "(9, 1, '', FROM_UNIXTIME(1700000400), '100')", + 'sample_row' => "(9, 0, '', FROM_UNIXTIME(1700000400), '100')", 'status_row' => null, ], thold_polling_sample_row($thold, ['traffic_in' => 700], '', 1700000300)); CactiStubs::reset(); $this->assertTrue(thold_daemon_persist_sample($thold, ['traffic_in' => 700], '', 1700000300)); $call = end(CactiStubs::$calls); - $this->assertSame(['', 1700000400, 100, 9], $call['params']); + $this->assertSame([0, '', 1700000400, 100, 9], $call['params']); } /** @@ -441,12 +476,12 @@ public function testDaemonPersistsThePairWithBoundParameters(): void { $this->assertTrue(thold_daemon_persist_sample($thold, [], '', 1300)); $call = end(CactiStubs::$calls); $this->assertStringContainsString('lasttime = FROM_UNIXTIME(?)', $call['sql']); - $this->assertSame(['', 1000, 100, 9], $call['params']); + $this->assertSame([0, '', 1000, 100, 9], $call['params']); CactiStubs::reset(); $this->assertTrue(thold_daemon_persist_sample($thold, ['traffic_in' => 700], 2, 1600)); $call = end(CactiStubs::$calls); - $this->assertSame([2, 1600, 700, 9], $call['params']); + $this->assertSame([1, 2, 1600, 700, 9], $call['params']); } /** @@ -485,11 +520,11 @@ public function testNeverSampledThresholdLeavesTheTimestampPairUntouched(): void $call = end(CactiStubs::$calls); $this->assertStringNotContainsString('FROM_UNIXTIME', $call['sql']); $this->assertStringNotContainsString('oldvalue', $call['sql']); - $this->assertSame(['', 9], $call['params']); + $this->assertSame([0, '', 9], $call['params']); CactiStubs::reset(); $this->assertSame( - ['sample_row' => null, 'status_row' => "(9, 1, '')"], + ['sample_row' => null, 'status_row' => "(9, 0, '')"], thold_polling_sample_row($thold, ['traffic_in' => 'U'], '', 1300) ); $this->assertSame([], CactiStubs::$calls); diff --git a/thold_functions.php b/thold_functions.php index 0c45e8f8..22894f36 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -842,7 +842,8 @@ function thold_sample_persistence(array $thold_data, array $item, $currenttime) * @return bool */ function thold_daemon_persist_sample(array $thold_data, array $item, $currentval, $currenttime) { - $id = (int) ($thold_data['thold_id'] ?? 0); + $id = (int) ($thold_data['thold_id'] ?? 0); + $tcheck = is_numeric($currentval) ? 1 : 0; if ($id <= 0) { return false; @@ -852,16 +853,16 @@ function thold_daemon_persist_sample(array $thold_data, array $item, $currentval if ($sample['lasttime'] <= 0) { return db_execute_prepared('UPDATE thold_data - SET tcheck = 1, lastread = ? + SET tcheck = ?, lastread = ? WHERE id = ?', - [$currentval, $id]); + [$tcheck, $currentval, $id]); } return db_execute_prepared('UPDATE thold_data - SET tcheck = 1, lastread = ?, + SET tcheck = ?, lastread = ?, lasttime = FROM_UNIXTIME(?), oldvalue = ? WHERE id = ?', - [$currentval, $sample['lasttime'], $sample['oldvalue'], $id]); + [$tcheck, $currentval, $sample['lasttime'], $sample['oldvalue'], $id]); } /** @@ -875,7 +876,8 @@ function thold_daemon_persist_sample(array $thold_data, array $item, $currentval * @return array{sample_row:string|null,status_row:string|null} */ function thold_polling_sample_row(array $thold_data, array $item, $currentval, $currenttime) { - $id = (int) ($thold_data['id'] ?? 0); + $id = (int) ($thold_data['id'] ?? 0); + $tcheck = is_numeric($currentval) ? 1 : 0; if ($id <= 0) { return ['sample_row' => null, 'status_row' => null]; @@ -886,12 +888,12 @@ function thold_polling_sample_row(array $thold_data, array $item, $currentval, $ if ($sample['lasttime'] <= 0) { return [ 'sample_row' => null, - 'status_row' => '(' . $id . ', 1, ' . db_qstr($currentval) . ')', + 'status_row' => '(' . $id . ', ' . $tcheck . ', ' . db_qstr($currentval) . ')', ]; } return [ - 'sample_row' => '(' . $id . ', 1, ' . db_qstr($currentval) + 'sample_row' => '(' . $id . ', ' . $tcheck . ', ' . db_qstr($currentval) . ', FROM_UNIXTIME(' . $sample['lasttime'] . '), ' . db_qstr($sample['oldvalue']) . ')', 'status_row' => null, ]; @@ -945,10 +947,13 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe ? (float) $thold_data['rrd_step'] : 0.0; $sample_interval = max($rrd_step, (float) $poller_interval); + $rrd_heartbeat = is_numeric($thold_data['rrd_heartbeat'] ?? null) && $thold_data['rrd_heartbeat'] > 0 + ? (float) $thold_data['rrd_heartbeat'] + : 2 * $sample_interval; $previous_sample_usable = $thold_data['lasttime'] > 0 && is_numeric($elapsed_step) && $elapsed_step > 0 - && $elapsed_step <= 2 * $sample_interval; + && $elapsed_step <= $rrd_heartbeat; $currentval = ''; @@ -1007,7 +1012,7 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe $currentval = $currentval / $step; } } else { - $currentval = $thold_data['lasttime'] > 0 ? '' : 0; + $currentval = ''; } break; @@ -1015,7 +1020,7 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe if ($previous_sample_usable && is_numeric($thold_data['oldvalue'])) { $currentval = ($item[$thold_data['name']] - $thold_data['oldvalue']) / $step; } else { - $currentval = $thold_data['lasttime'] > 0 ? '' : 0; + $currentval = ''; } break; diff --git a/thold_process.php b/thold_process.php index 4422b4e7..7edf3e7d 100644 --- a/thold_process.php +++ b/thold_process.php @@ -209,7 +209,9 @@ thold_daemon_debug(sprintf('Checked Name:%s, Graph:%s, Value:%s, Time:%s', $thold_data['thold_name'], $thold_data['local_graph_id'], $currentval, $currenttime), $thread); - thold_daemon_persist_sample($thold_data, $item, $currentval, $currenttime); + if (!thold_daemon_persist_sample($thold_data, $item, $currentval, $currenttime)) { + thold_daemon_debug(sprintf('Failed to persist threshold sample for ID %s.', $thold_data['thold_id']), $thread); + } } $tholds = thold_get_thresholds_tholdcheck($thread, $start_time); @@ -357,7 +359,7 @@ function thold_get_thresholds_precheck($thread, $start_time) { td.data_template_rrd_id, td.lastread, UNIX_TIMESTAMP(td.lasttime) AS lasttime, td.oldvalue, dtr.data_source_name AS name, dtr.data_source_type_id, - dtd.rrd_step, dtr.rrd_maximum + dtd.rrd_step, dtr.rrd_maximum, dtr.rrd_heartbeat FROM plugin_thold_daemon_data AS tdd INNER JOIN thold_data AS td ON td.id = tdd.id @@ -378,7 +380,7 @@ function thold_get_thresholds_precheck($thread, $start_time) { td.data_template_rrd_id, td.lastread, UNIX_TIMESTAMP(td.lasttime) AS lasttime, td.oldvalue, dtr.data_source_name AS name, dtr.data_source_type_id, - dtd.rrd_step, dtr.rrd_maximum + dtd.rrd_step, dtr.rrd_maximum, dtr.rrd_heartbeat FROM plugin_thold_daemon_data AS tdd INNER JOIN thold_data AS td ON td.id = tdd.id From f0935eafc1d59f151bbe74a168c9b45d0494aac2 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 18 Aug 2026 00:43:41 -0700 Subject: [PATCH 13/17] fix: preserve unknown values through transforms --- tests/Unit/TholdGetCurrentvalTest.php | 39 +++++++++++++++++++++++---- thold_functions.php | 28 +++++++++++++------ 2 files changed, 54 insertions(+), 13 deletions(-) diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index a463f2b1..575e64f8 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -215,6 +215,28 @@ public function testLowerUpperCombinationRejectsUnknownInputs(): void { $this->assertSame((5 << 32) + 7, thold_calculate_lower_upper($thold, 7, [4 => ['upper' => 5]])); } + /** + * @return void + */ + public function testCdefAndNestedExpressionPreserveUnknownValues(): void { + $this->assertSame('', thold_build_cdef(1, '', 4, 5)); + $this->assertSame( + ['sample_row' => "(9, 0, '', FROM_UNIXTIME(1700000300), '700')", 'status_row' => null], + thold_polling_sample_row($this->threshold(), ['traffic_in' => 700], '', 1700000300) + ); + + $nested = $this->threshold([ + 'lasttime' => 1000, + 'rrd_heartbeat' => 600, + ]); + CactiStubs::willReturn('db_fetch_row_prepared', $nested); + $outer = $this->threshold(['expression' => '|ds:traffic_in|']); + $reindexed = [4 => ['traffic_in' => 700]]; + $time_reindexed = [4 => 1900]; + + $this->assertSame('', thold_calculate_expression($outer, '', $reindexed, $time_reindexed)); + } + /** * @return void */ @@ -376,7 +398,7 @@ public function testEvaluationCadenceMayExceedTheRrdStep(): void { /** * @return void */ - public function testRrdHeartbeatControlsGapAcceptance(): void { + public function testRrdHeartbeatControlsGapAcceptanceWithASafeFloor(): void { $accepted = $this->threshold(['lasttime' => 1000, 'rrd_heartbeat' => 1800]); $reindexed = [4 => ['traffic_in' => 700]]; $time_reindexed = [4 => 1900]; @@ -389,10 +411,17 @@ public function testRrdHeartbeatControlsGapAcceptance(): void { 1.0e-9 ); - $rejected = $accepted; - $rejected['rrd_heartbeat'] = 300; - $time_reindexed[4] = 1400; - $this->assertSame('', thold_get_currentval($rejected, $reindexed, $time_reindexed, $item, $currenttime)); + $floored = $accepted; + $floored['rrd_heartbeat'] = 120; + $time_reindexed[4] = 1300; + $this->assertEqualsWithDelta( + 2, + thold_get_currentval($floored, $reindexed, $time_reindexed, $item, $currenttime), + 1.0e-9 + ); + + $time_reindexed[4] = 1700; + $this->assertSame('', thold_get_currentval($floored, $reindexed, $time_reindexed, $item, $currenttime)); } /** diff --git a/thold_functions.php b/thold_functions.php index 22894f36..b218d1b1 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -948,13 +948,21 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe : 0.0; $sample_interval = max($rrd_step, (float) $poller_interval); $rrd_heartbeat = is_numeric($thold_data['rrd_heartbeat'] ?? null) && $thold_data['rrd_heartbeat'] > 0 - ? (float) $thold_data['rrd_heartbeat'] + ? max((float) $thold_data['rrd_heartbeat'], 2 * $sample_interval) : 2 * $sample_interval; $previous_sample_usable = $thold_data['lasttime'] > 0 && is_numeric($elapsed_step) && $elapsed_step > 0 && $elapsed_step <= $rrd_heartbeat; + if ($thold_data['lasttime'] > 0 && is_numeric($elapsed_step) && $elapsed_step > $rrd_heartbeat && function_exists('thold_debug')) { + thold_debug(sprintf( + 'Threshold sample gap of %s seconds exceeds the effective heartbeat of %s seconds.', + $elapsed_step, + $rrd_heartbeat + ), 'thold'); + } + $currentval = ''; if (isset($rrd_reindexed[$thold_data['local_data_id']])) { @@ -1090,7 +1098,7 @@ function thold_calculate_expression($thold, $currentval, &$rrd_reindexed, &$rrd_ td.host_id, td.cdef, td.local_data_id, td.data_template_rrd_id, td.lastread, UNIX_TIMESTAMP(td.lasttime) AS lasttime, td.oldvalue, dtr.data_source_name as name, - dtr.data_source_type_id, dtd.rrd_step, dtr.rrd_maximum + dtr.data_source_type_id, dtd.rrd_step, dtr.rrd_maximum, dtr.rrd_heartbeat FROM thold_data AS td LEFT JOIN data_template_rrd AS dtr ON dtr.id = td.data_template_rrd_id @@ -1106,6 +1114,10 @@ function thold_calculate_expression($thold, $currentval, &$rrd_reindexed, &$rrd_ $item = []; $currenttime = 0; $value = thold_get_currentval($thold_item, $rrd_reindexed, $rrd_time_reindexed, $item, $currenttime); + + if (!is_numeric($value)) { + return ''; + } } // Previous returns 'U' after device recovers. Try alternate @@ -1123,11 +1135,11 @@ function thold_calculate_expression($thold, $currentval, &$rrd_reindexed, &$rrd_ } } - $expression[$key] = $value; - - if ($expression[$key] == '') { - $expression[$key] = '0'; + if (!is_numeric($value)) { + return ''; } + + $expression[$key] = $value; } elseif (strpos($item, '|') !== false) { // Remove invalid characters $item = str_replace('\\', '', $item); @@ -4794,8 +4806,8 @@ function thold_cdef_select_usable_names() { } function thold_build_cdef($cdef, $value, $local_data_id, $data_template_rrd_id) { - if ($value == '') { - $value = 0; + if (!is_numeric($value)) { + return ''; } $oldvalue = $value; From 5443ce73fa464c5e4069401036c2a43c81ed1ac2 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 18 Aug 2026 00:44:50 -0700 Subject: [PATCH 14/17] fix: fail closed on missing expression sources --- tests/Unit/TholdGetCurrentvalTest.php | 9 +++++++++ thold_functions.php | 29 +++++---------------------- 2 files changed, 14 insertions(+), 24 deletions(-) diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index 575e64f8..a2ccb1f8 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -235,6 +235,15 @@ public function testCdefAndNestedExpressionPreserveUnknownValues(): void { $time_reindexed = [4 => 1900]; $this->assertSame('', thold_calculate_expression($outer, '', $reindexed, $time_reindexed)); + + CactiStubs::reset(); + CactiStubs::willReturn('db_fetch_row_prepared', $nested); + $time_reindexed[4] = 1300; + $this->assertEqualsWithDelta(2, thold_calculate_expression($outer, '', $reindexed, $time_reindexed), 1.0e-9); + + CactiStubs::reset(); + CactiStubs::willReturn('db_fetch_row_prepared', []); + $this->assertSame('', thold_calculate_expression($outer, '', $reindexed, $time_reindexed)); } /** diff --git a/thold_functions.php b/thold_functions.php index b218d1b1..245b2755 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -1108,32 +1108,13 @@ function thold_calculate_expression($thold, $currentval, &$rrd_reindexed, &$rrd_ AND td.local_data_id = ?', [$dsname, $thold['local_data_id']]); - $value = ''; - - if (cacti_sizeof($thold_item)) { - $item = []; - $currenttime = 0; - $value = thold_get_currentval($thold_item, $rrd_reindexed, $rrd_time_reindexed, $item, $currenttime); - - if (!is_numeric($value)) { - return ''; - } + if (!cacti_sizeof($thold_item)) { + return ''; } - // Previous returns 'U' after device recovers. Try alternate - if (empty($value) || $value == 'U') { - if (read_config_option('dsstats_enable') == 'on') { - $value = db_fetch_cell_prepared('SELECT calculated - FROM data_source_stats_hourly_last - WHERE local_data_id = ? - AND rrd_name = ?', - [$thold['local_data_id'], $dsname]); - } - - if (empty($value) || $value == 'U' || $value == '-90909090909') { - $value = get_current_value($thold['local_data_id'], $dsname); - } - } + $item = []; + $currenttime = 0; + $value = thold_get_currentval($thold_item, $rrd_reindexed, $rrd_time_reindexed, $item, $currenttime); if (!is_numeric($value)) { return ''; From 10c83d5019eb0cca4824752b91b2d9f065c30899 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 18 Aug 2026 01:00:51 -0700 Subject: [PATCH 15/17] fix: keep unavailable thresholds eligible --- CHANGELOG.md | 2 +- README.md | 13 ++- includes/polling.php | 16 ++- tests/Unit/GetCurrentValueTest.php | 12 ++ tests/Unit/TholdGetCurrentvalTest.php | 100 +++++++++++++++-- .../ThresholdHiLowCharacterizationTest.php | 7 +- ...ThresholdTimeBasedCharacterizationTest.php | 7 +- tests/bootstrap-unit.php | 5 + thold_functions.php | 105 ++++++++++++++---- 9 files changed, 228 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42b97ce9..9fbdaad2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ * 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#815: Keep counter sample values and timestamps synchronized +* issue#815: Keep counter sample values and timestamps synchronized, recover from backward sample clocks, preserve alert state while samples are unavailable, and fail closed when expression sources cannot be resolved * issue: All Columns checkd on Thresholds page * issue: Special character previous value handling broken on data query indexes with special characters diff --git a/README.md b/README.md index 9cb3ac66..70bba5db 100644 --- a/README.md +++ b/README.md @@ -32,9 +32,16 @@ timestamp together when a poll has no numeric sample, preventing the next rate from using mismatched interval data. A gap of more than two effective sampling intervals (the greater of the RRD step and poller interval), or the configured RRD heartbeat when available, is treated as unknown while the current sample -starts a fresh baseline instead of producing a stale rate. Unknown rates are -not evaluated, so existing alert state is preserved. Gauge readings remain -valid across such a gap. +starts a fresh baseline instead of producing a stale rate. Thresholds with an +unknown rate remain eligible for the next poll, but that poll preserves the +existing alert state and writes a warning instead of treating the missing +sample as a restoral. Warnings are emitted only when the threshold enters the +unavailable state. A backward sample clock is re-anchored without calculating a +rate for that cycle, so later samples can recover normally. Expression +thresholds require a numeric sibling value in the current poll and use its +RRD-calculated value even when the sibling has no threshold row, preserving +COUNTER and DERIVE rate units. They fail closed when the current sibling value +is unavailable. Gauge readings remain valid across such a gap. 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 diff --git a/includes/polling.php b/includes/polling.php index c7a82e26..35afde9a 100644 --- a/includes/polling.php +++ b/includes/polling.php @@ -234,12 +234,22 @@ function thold_poller_output(&$rrd_update_array) { if (cacti_sizeof($status_sql)) { foreach (array_chunk($status_sql, 400) as $chunk) { - db_execute('INSERT INTO thold_data + $placeholders = implode(', ', array_fill(0, cacti_sizeof($chunk), '(?, ?, ?)')); + $params = []; + + foreach ($chunk as $row) { + $params[] = $row['id']; + $params[] = $row['tcheck']; + $params[] = $row['lastread']; + } + + db_execute_prepared('INSERT INTO thold_data (id, tcheck, lastread) - VALUES ' . implode(', ', $chunk) . ' + VALUES ' . $placeholders . ' ON DUPLICATE KEY UPDATE tcheck = VALUES(tcheck), - lastread = VALUES(lastread)'); + lastread = VALUES(lastread)', + $params); } } diff --git a/tests/Unit/GetCurrentValueTest.php b/tests/Unit/GetCurrentValueTest.php index f467b9bc..85c0a087 100644 --- a/tests/Unit/GetCurrentValueTest.php +++ b/tests/Unit/GetCurrentValueTest.php @@ -96,6 +96,12 @@ public function testMissingDataSourceNamesReturnsZero(): void { $this->rrdReturns([]); $this->assertSame(0, get_current_value(4, 'traffic_in')); + + CactiStubs::reset(); + CactiStubs::willReturn('db_fetch_row_prepared', ['rrd_step' => 300]); + CactiStubs::willReturn('rrdtool_execute', '1700000000'); + $this->rrdReturns([]); + $this->assertSame('', get_current_value(4, 'traffic_in', 0, '')); } /** @@ -105,6 +111,12 @@ public function testMissingValuesReturnsZero(): void { $this->rrdReturns(['data_source_names' => ['traffic_in']]); $this->assertSame(0, get_current_value(4, 'traffic_in')); + + CactiStubs::reset(); + CactiStubs::willReturn('db_fetch_row_prepared', ['rrd_step' => 300]); + CactiStubs::willReturn('rrdtool_execute', '1700000000'); + $this->rrdReturns(['data_source_names' => ['traffic_out'], 'values' => [['1700000000' => 20.0]]]); + $this->assertSame('', get_current_value(4, 'traffic_in', 0, '')); } /** diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index a2ccb1f8..6de34497 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -212,7 +212,12 @@ public function testLowerUpperCombinationRejectsUnknownInputs(): void { $this->assertSame('', thold_calculate_lower_upper($thold, '', [4 => ['upper' => 5]])); $this->assertSame('', thold_calculate_lower_upper($thold, 7, [4 => ['upper' => 'U']])); - $this->assertSame((5 << 32) + 7, thold_calculate_lower_upper($thold, 7, [4 => ['upper' => 5]])); + $this->assertSame('', thold_calculate_lower_upper($thold, 7, [4 => []])); + $this->assertEqualsWithDelta((5 * 4294967296) + 7, thold_calculate_lower_upper($thold, 7, [4 => ['upper' => 5]]), 1.0e-9); + $this->assertEqualsWithDelta((2147483648.0 * 4294967296) + 7, thold_calculate_lower_upper($thold, 7, [4 => ['upper' => 2147483648]]), 1); + $this->assertEqualsWithDelta((4294967295.0 * 4294967296) + 7, thold_calculate_lower_upper($thold, 7, [4 => ['upper' => 4294967295]]), 1); + $this->assertSame('', thold_calculate_lower_upper($thold, 7, [4 => ['upper' => -1]])); + $this->assertSame('', thold_calculate_lower_upper($thold, 7, [4 => ['upper' => 4294967296]])); } /** @@ -221,7 +226,7 @@ public function testLowerUpperCombinationRejectsUnknownInputs(): void { public function testCdefAndNestedExpressionPreserveUnknownValues(): void { $this->assertSame('', thold_build_cdef(1, '', 4, 5)); $this->assertSame( - ['sample_row' => "(9, 0, '', FROM_UNIXTIME(1700000300), '700')", 'status_row' => null], + ['sample_row' => "(9, 1, '', FROM_UNIXTIME(1700000300), '700')", 'status_row' => null], thold_polling_sample_row($this->threshold(), ['traffic_in' => 700], '', 1700000300) ); @@ -230,11 +235,12 @@ public function testCdefAndNestedExpressionPreserveUnknownValues(): void { 'rrd_heartbeat' => 600, ]); CactiStubs::willReturn('db_fetch_row_prepared', $nested); - $outer = $this->threshold(['expression' => '|ds:traffic_in|']); + $outer = $this->threshold(['expression' => '|ds:traffic_in|', 'lastread' => 2]); $reindexed = [4 => ['traffic_in' => 700]]; $time_reindexed = [4 => 1900]; $this->assertSame('', thold_calculate_expression($outer, '', $reindexed, $time_reindexed)); + $this->assertSame([], CactiStubs::$log); CactiStubs::reset(); CactiStubs::willReturn('db_fetch_row_prepared', $nested); @@ -243,7 +249,48 @@ public function testCdefAndNestedExpressionPreserveUnknownValues(): void { CactiStubs::reset(); CactiStubs::willReturn('db_fetch_row_prepared', []); + CactiStubs::willReturn('db_fetch_row_prepared', ['data_source_type_id' => self::COUNTER]); + CactiStubs::willReturn('db_fetch_row_prepared', ['rrd_step' => 300]); + CactiStubs::willReturn('rrdtool_execute', '1700000000'); + CactiStubs::willReturn('rrdtool_function_fetch', [ + 'data_source_names' => ['traffic_in'], + 'values' => [['1700000000' => 2.0]], + ]); + $this->assertEqualsWithDelta(2.0, thold_calculate_expression($outer, '', $reindexed, $time_reindexed), 1.0e-9); + $this->assertSame([], CactiStubs::$log); + + CactiStubs::reset(); + CactiStubs::willReturn('db_fetch_row_prepared', []); + CactiStubs::willReturn('db_fetch_row_prepared', ['data_source_type_id' => self::GAUGE]); + $this->assertSame('700', thold_calculate_expression($outer, '', $reindexed, $time_reindexed)); + $this->assertSame([], CactiStubs::callsTo('rrdtool_function_fetch')); + + foreach ([ + [], + ['data_source_names' => ['traffic_out'], 'values' => [['1700000000' => 2.0]]], + ] as $missing_fetch) { + CactiStubs::reset(); + CactiStubs::willReturn('db_fetch_row_prepared', []); + CactiStubs::willReturn('db_fetch_row_prepared', ['data_source_type_id' => self::COUNTER]); + CactiStubs::willReturn('db_fetch_row_prepared', ['rrd_step' => 300]); + CactiStubs::willReturn('rrdtool_execute', '1700000000'); + CactiStubs::willReturn('rrdtool_function_fetch', $missing_fetch); + $this->assertSame('', thold_calculate_expression($outer, '', $reindexed, $time_reindexed)); + } + + CactiStubs::reset(); + CactiStubs::willReturn('db_fetch_row_prepared', []); + CactiStubs::willReturn('db_fetch_row_prepared', []); + $this->assertSame('', thold_calculate_expression($outer, '', $reindexed, $time_reindexed)); + + CactiStubs::reset(); + CactiStubs::willReturn('db_fetch_row_prepared', []); + $reindexed = []; $this->assertSame('', thold_calculate_expression($outer, '', $reindexed, $time_reindexed)); + $this->assertStringContainsString('expression source traffic_in is unavailable', CactiStubs::$log[0]); + $log_call = CactiStubs::callsTo('cacti_log')[0]; + $this->assertSame('THOLD', $log_call['params'][2]); + $this->assertSame(POLLER_VERBOSITY_MEDIUM, $log_call['params'][3]); } /** @@ -288,8 +335,8 @@ public static function emptyMaximumProvider() { 'integer zero' => [0, 1009000000100, 1009000000100 / 600], 'empty string' => ['', 1009000000100, 1009000000100 / 600], 'null maximum' => [null, 1009000000100, 1009000000100 / 600], - 'unknown maximum' => ['U', 1009000000100, 9000000100 / 600], - 'unresolved if speed' => ['|query_ifSpeed|', 1009000000100, 9000000100 / 600], + 'unknown maximum' => ['U', 1009000000100, 1009000000100 / 600], + 'unresolved if speed' => ['|query_ifSpeed|', 1009000000100, 1009000000100 / 600], 'explicit maximum' => [20000000, 1015000000000, 1015000000000 / 600], ]; } @@ -475,7 +522,7 @@ public function testOutOfOrderCounterAndDeriveSamplesAreUnknown(): void { } $this->assertSame( - ['lasttime' => 1700000400, 'oldvalue' => 100], + ['lasttime' => 1700000300, 'oldvalue' => 700], thold_sample_persistence($thold, ['traffic_in' => 700], 1700000300) ); $this->assertSame( @@ -483,14 +530,25 @@ public function testOutOfOrderCounterAndDeriveSamplesAreUnknown(): void { thold_sample_persistence($thold, ['traffic_in' => 700], 1700000400) ); $this->assertSame([ - 'sample_row' => "(9, 0, '', FROM_UNIXTIME(1700000400), '100')", + 'sample_row' => "(9, 1, '', FROM_UNIXTIME(1700000300), '700')", 'status_row' => null, ], thold_polling_sample_row($thold, ['traffic_in' => 700], '', 1700000300)); CactiStubs::reset(); $this->assertTrue(thold_daemon_persist_sample($thold, ['traffic_in' => 700], '', 1700000300)); $call = end(CactiStubs::$calls); - $this->assertSame([0, '', 1700000400, 100, 9], $call['params']); + $this->assertSame([1, '', 1700000300, 700, 9], $call['params']); + + $reanchored = $this->threshold(['lasttime' => 1700000300, 'oldvalue' => 700]); + $reindexed = [4 => ['traffic_in' => 1000]]; + $time_reindexed = [4 => 1700000600]; + $item = []; + $currenttime = 0; + $this->assertEqualsWithDelta( + 1, + thold_get_currentval($reanchored, $reindexed, $time_reindexed, $item, $currenttime), + 1.0e-9 + ); } /** @@ -514,7 +572,7 @@ public function testDaemonPersistsThePairWithBoundParameters(): void { $this->assertTrue(thold_daemon_persist_sample($thold, [], '', 1300)); $call = end(CactiStubs::$calls); $this->assertStringContainsString('lasttime = FROM_UNIXTIME(?)', $call['sql']); - $this->assertSame([0, '', 1000, 100, 9], $call['params']); + $this->assertSame([1, '', 1000, 100, 9], $call['params']); CactiStubs::reset(); $this->assertTrue(thold_daemon_persist_sample($thold, ['traffic_in' => 700], 2, 1600)); @@ -558,11 +616,11 @@ public function testNeverSampledThresholdLeavesTheTimestampPairUntouched(): void $call = end(CactiStubs::$calls); $this->assertStringNotContainsString('FROM_UNIXTIME', $call['sql']); $this->assertStringNotContainsString('oldvalue', $call['sql']); - $this->assertSame([0, '', 9], $call['params']); + $this->assertSame([1, '', 9], $call['params']); CactiStubs::reset(); $this->assertSame( - ['sample_row' => null, 'status_row' => "(9, 0, '')"], + ['sample_row' => null, 'status_row' => ['id' => 9, 'tcheck' => 1, 'lastread' => '']], thold_polling_sample_row($thold, ['traffic_in' => 'U'], '', 1300) ); $this->assertSame([], CactiStubs::$calls); @@ -603,6 +661,26 @@ public function testMissingPersistenceKeysFailClosed(): void { ); } + /** + * @return void + */ + public function testUnavailableSampleLogsOnlyOnTheStateTransition(): void { + $thold = $this->threshold([ + 'lastread' => 12, + 'name_cache' => 'Traffic in', + ]); + + thold_polling_sample_row($thold, [], '', 1700000300); + $this->assertCount(1, CactiStubs::$log); + $log_call = CactiStubs::callsTo('cacti_log')[0]; + $this->assertSame('THOLD', $log_call['params'][2]); + $this->assertSame(POLLER_VERBOSITY_MEDIUM, $log_call['params'][3]); + + $thold['lastread'] = ''; + thold_polling_sample_row($thold, [], '', 1700000600); + $this->assertCount(1, CactiStubs::$log); + } + /** * @return void */ diff --git a/tests/Unit/ThresholdHiLowCharacterizationTest.php b/tests/Unit/ThresholdHiLowCharacterizationTest.php index 85eb1004..398230ae 100644 --- a/tests/Unit/ThresholdHiLowCharacterizationTest.php +++ b/tests/Unit/ThresholdHiLowCharacterizationTest.php @@ -237,9 +237,14 @@ public function testMaintenanceWindowSuppressesNotification(): void { * @return void */ public function testUnknownReadingEmitsNoAlert(): void { - $outcome = $this->bounded(['lastread' => 'U'])->poll(); + $outcome = $this->bounded([ + 'lastread' => 'U', + 'thold_alert' => 2, + 'thold_fail_count' => 3, + ])->poll(); $this->assertSame(0, $outcome->mailCount()); + $this->assertNull($outcome->persistedAlertState()); } /** diff --git a/tests/Unit/ThresholdTimeBasedCharacterizationTest.php b/tests/Unit/ThresholdTimeBasedCharacterizationTest.php index e5a87fd5..b76e8507 100644 --- a/tests/Unit/ThresholdTimeBasedCharacterizationTest.php +++ b/tests/Unit/ThresholdTimeBasedCharacterizationTest.php @@ -134,9 +134,14 @@ public function testAcknowledgedThresholdDoesNotMailOnBreach(): void { * @return void */ public function testUnknownReadingEmitsNoAlert(): void { - $outcome = $this->bounded(['lastread' => 'U'])->poll(); + $outcome = $this->bounded([ + 'lastread' => 'U', + 'thold_alert' => 2, + 'thold_fail_count' => 3, + ])->poll(); $this->assertSame(0, $outcome->mailCount()); + $this->assertNull($outcome->persistedAlertState()); } /** diff --git a/tests/bootstrap-unit.php b/tests/bootstrap-unit.php index cdb9f015..e0b2991f 100644 --- a/tests/bootstrap-unit.php +++ b/tests/bootstrap-unit.php @@ -65,6 +65,10 @@ require_once __DIR__ . '/Helpers/ThresholdOutcome.php'; require_once __DIR__ . '/Helpers/ThresholdScenario.php'; +if (!defined('POLLER_VERBOSITY_MEDIUM')) { + define('POLLER_VERBOSITY_MEDIUM', 3); +} + /* * base_path has to point at the Cacti root two levels above this plugin: * thold_functions.php builds include paths from it at runtime. @@ -250,6 +254,7 @@ function __esc($text) { if (!function_exists('cacti_log')) { function cacti_log($message, $output = false, $environ = 'CMDPHP', $level = 0) { CactiStubs::$log[] = $message; + CactiStubs::record('cacti_log', '', [$message, $output, $environ, $level]); } } diff --git a/thold_functions.php b/thold_functions.php index 245b2755..a6bc5549 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -821,7 +821,14 @@ function thold_sample_persistence(array $thold_data, array $item, $currenttime) $lasttime = (int) ($thold_data['lasttime'] ?? 0); - if ($name !== '' && $currenttime > $lasttime && isset($item[$name]) && is_numeric($item[$name])) { + if ($name !== '' && $currenttime > 0 && $currenttime !== $lasttime && isset($item[$name]) && is_numeric($item[$name])) { + if ($lasttime > 0 && $currenttime < $lasttime) { + cacti_log(sprintf( + 'WARNING: Threshold %s sample clock moved backwards; re-anchoring its value and timestamp.', + $thold_data['id'] ?? ($thold_data['thold_id'] ?? 'unknown') + ), false, 'THOLD', POLLER_VERBOSITY_MEDIUM); + } + return ['lasttime' => $currenttime, 'oldvalue' => $item[$name]]; } @@ -831,6 +838,26 @@ function thold_sample_persistence(array $thold_data, array $item, $currenttime) ]; } +/** + * Log only the transition from a numeric result to an unavailable result. + * + * @param array $thold_data + * @param mixed $currentval + * + * @return void + */ +function thold_log_unavailable_transition(array $thold_data, $currentval) { + if (is_numeric($currentval) || !is_numeric($thold_data['lastread'] ?? null)) { + return; + } + + cacti_log(sprintf( + 'WARNING: Threshold %s (%s) current sample is unavailable; preserving its alert state.', + $thold_data['id'] ?? ($thold_data['thold_id'] ?? 'unknown'), + $thold_data['name_cache'] ?? ($thold_data['thold_name'] ?? ($thold_data['name'] ?? 'unknown')) + ), false, 'THOLD', POLLER_VERBOSITY_MEDIUM); +} + /** * Persist one daemon sample without manufacturing a zero SQL timestamp. * @@ -843,12 +870,14 @@ function thold_sample_persistence(array $thold_data, array $item, $currenttime) */ function thold_daemon_persist_sample(array $thold_data, array $item, $currentval, $currenttime) { $id = (int) ($thold_data['thold_id'] ?? 0); - $tcheck = is_numeric($currentval) ? 1 : 0; + $tcheck = 1; if ($id <= 0) { return false; } + thold_log_unavailable_transition($thold_data, $currentval); + $sample = thold_sample_persistence($thold_data, $item, $currenttime); if ($sample['lasttime'] <= 0) { @@ -873,22 +902,24 @@ function thold_daemon_persist_sample(array $thold_data, array $item, $currentval * @param mixed $currentval * @param int $currenttime * - * @return array{sample_row:string|null,status_row:string|null} + * @return array{sample_row:string|null,status_row:array{id:int,tcheck:int,lastread:mixed}|null} */ function thold_polling_sample_row(array $thold_data, array $item, $currentval, $currenttime) { $id = (int) ($thold_data['id'] ?? 0); - $tcheck = is_numeric($currentval) ? 1 : 0; + $tcheck = 1; if ($id <= 0) { return ['sample_row' => null, 'status_row' => null]; } + thold_log_unavailable_transition($thold_data, $currentval); + $sample = thold_sample_persistence($thold_data, $item, $currenttime); if ($sample['lasttime'] <= 0) { return [ 'sample_row' => null, - 'status_row' => '(' . $id . ', ' . $tcheck . ', ' . db_qstr($currentval) . ')', + 'status_row' => ['id' => $id, 'tcheck' => $tcheck, 'lastread' => $currentval], ]; } @@ -1007,9 +1038,7 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe } $maximum_value = $thold_data['rrd_maximum'] ?? ''; - $rrd_maximum = trim((string) $maximum_value) === '' - ? 0.0 - : (is_numeric($maximum_value) ? (float) $maximum_value : null); + $rrd_maximum = is_numeric($maximum_value) ? (float) $maximum_value : 0.0; // assume counter reset if greater than max value if ($rrd_maximum > 0 && ($currentval / $step) > $rrd_maximum) { @@ -1074,7 +1103,7 @@ function thold_calculate_expression($thold, $currentval, &$rrd_reindexed, &$rrd_ $expression = explode(',', $thold['expression']); // out current data sources - $data_sources = $rrd_reindexed[$thold['local_data_id']]; + $data_sources = $rrd_reindexed[$thold['local_data_id']] ?? []; if (cacti_sizeof($data_sources)) { foreach ($data_sources as $key => $value) { @@ -1109,6 +1138,39 @@ function thold_calculate_expression($thold, $currentval, &$rrd_reindexed, &$rrd_ [$dsname, $thold['local_data_id']]); if (!cacti_sizeof($thold_item)) { + $current_sample = $rrd_reindexed[$thold['local_data_id']][$dsname] ?? ''; + + if (is_numeric($current_sample)) { + $source = db_fetch_row_prepared('SELECT data_source_type_id + FROM data_template_rrd + WHERE local_data_id = ? + AND data_source_name = ?', + [$thold['local_data_id'], $dsname]); + + if (cacti_sizeof($source) && $source['data_source_type_id'] == 1) { + $value = $current_sample; + } elseif (cacti_sizeof($source)) { + $value = get_current_value($thold['local_data_id'], $dsname, 0, ''); + } else { + $value = ''; + } + + if (is_numeric($value)) { + $expression[$key] = $value; + + continue; + } + } + + if (is_numeric($thold['lastread'] ?? null)) { + cacti_log(sprintf( + 'WARNING: Threshold %s expression source %s is unavailable for local data ID %s.', + $thold['id'] ?? 'unknown', + $dsname, + $thold['local_data_id'] ?? 'unknown' + ), false, 'THOLD', POLLER_VERBOSITY_MEDIUM); + } + return ''; } @@ -1390,17 +1452,17 @@ function thold_calculate_lower_upper($thold, $currentval, $rrd_reindexed) { return ''; } - if (isset($rrd_reindexed[$thold['local_data_id']][$ds])) { - $t = $rrd_reindexed[$thold['local_data_id']][$thold['upper_ds']]; + if (!isset($rrd_reindexed[$thold['local_data_id']][$ds])) { + return ''; + } - if (!is_numeric($t)) { - return ''; - } + $t = $rrd_reindexed[$thold['local_data_id']][$thold['upper_ds']]; - $currentval = ((int) $t << 32) + $currentval; + if (!is_numeric($t) || $t < 0 || $t > 4294967295) { + return ''; } - return $currentval; + return ((float) $t * 4294967296) + $currentval; } function get_allowed_thresholds($sql_where = '', $order_by = 'td.name', $sql_limit = '', &$total_rows = 0, $user_id = 0, $graph_id = 0) { @@ -2434,6 +2496,11 @@ function thold_check_threshold(&$thold_data) { return; } + // An unavailable sample is not evidence that an active alert recovered. + if (!is_numeric($thold_data['lastread'])) { + return; + } + $alert_exempt = read_config_option('alert_exempt'); // check for exemptions $weekday = date('l'); @@ -5042,7 +5109,7 @@ function thold_rrd_last($local_data_id) { return trim($last_time_entry); } -function get_current_value($local_data_id, $data_template_rrd_id, $cdef = 0) { +function get_current_value($local_data_id, $data_template_rrd_id, $cdef = 0, $missing_value = 0) { // get the information to populate into the rrd files if (function_exists('boost_check_correct_enabled') && boost_check_correct_enabled()) { boost_process_poller_output($local_data_id); @@ -5071,7 +5138,7 @@ function get_current_value($local_data_id, $data_template_rrd_id, $cdef = 0) { // Return Blank if the data source is not found (Newly created?) if (!isset($result['data_source_names'])) { - return 0; + return $missing_value; } // array_search() reports a miss as false. Testing for null let the miss @@ -5081,7 +5148,7 @@ function get_current_value($local_data_id, $data_template_rrd_id, $cdef = 0) { // Return Blank if the value was not found (Cache Cleared?) if ($idx === false || !isset($result['values'][$idx]) || !cacti_sizeof($result['values'][$idx])) { - return 0; + return $missing_value; } $value = array_values($result['values'][$idx])[0]; From 0ea5a266cfac2bf3741c0e789aaff7e730c64282 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 18 Aug 2026 02:18:49 -0700 Subject: [PATCH 16/17] fix: preserve cached expression rates --- README.md | 8 +++++--- tests/Unit/TholdCalculatePercentTest.php | 4 ++-- tests/Unit/TholdGetCurrentvalTest.php | 8 ++++++++ thold_functions.php | 20 ++++++++++++++++++-- 4 files changed, 33 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 70bba5db..2c208fbc 100644 --- a/README.md +++ b/README.md @@ -39,9 +39,11 @@ sample as a restoral. Warnings are emitted only when the threshold enters the unavailable state. A backward sample clock is re-anchored without calculating a rate for that cycle, so later samples can recover normally. Expression thresholds require a numeric sibling value in the current poll and use its -RRD-calculated value even when the sibling has no threshold row, preserving -COUNTER and DERIVE rate units. They fail closed when the current sibling value -is unavailable. Gauge readings remain valid across such a gap. +cached DSStats rate, with an RRD fallback, even when the sibling has no +threshold row. This preserves COUNTER and DERIVE rate units without adding an +rrdtool process per expression during normal operation. They fail closed when +the current sibling value is unavailable. Gauge readings remain valid across +such a gap. 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 diff --git a/tests/Unit/TholdCalculatePercentTest.php b/tests/Unit/TholdCalculatePercentTest.php index 4266fa39..3231ff32 100644 --- a/tests/Unit/TholdCalculatePercentTest.php +++ b/tests/Unit/TholdCalculatePercentTest.php @@ -80,8 +80,8 @@ public function testZeroDenominatorGivesZeroRatherThanDividingByZero(): void { /** * @return void */ - public function testNonNumericDenominatorGivesZero(): void { - $this->assertSame(0, $this->percent('U')); + public function testNonNumericDenominatorYieldsTheNoValueSentinel(): void { + $this->assertSame('', $this->percent('U')); } /** diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index 6de34497..c52fe43f 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -259,6 +259,14 @@ public function testCdefAndNestedExpressionPreserveUnknownValues(): void { $this->assertEqualsWithDelta(2.0, thold_calculate_expression($outer, '', $reindexed, $time_reindexed), 1.0e-9); $this->assertSame([], CactiStubs::$log); + CactiStubs::reset(); + CactiStubs::$configOptions['dsstats_enable'] = 'on'; + CactiStubs::willReturn('db_fetch_row_prepared', []); + CactiStubs::willReturn('db_fetch_row_prepared', ['data_source_type_id' => self::COUNTER]); + CactiStubs::willReturn('db_fetch_cell_prepared', 3.5); + $this->assertEqualsWithDelta(3.5, thold_calculate_expression($outer, '', $reindexed, $time_reindexed), 1.0e-9); + $this->assertSame([], CactiStubs::callsTo('rrdtool_function_fetch')); + CactiStubs::reset(); CactiStubs::willReturn('db_fetch_row_prepared', []); CactiStubs::willReturn('db_fetch_row_prepared', ['data_source_type_id' => self::GAUGE]); diff --git a/thold_functions.php b/thold_functions.php index bcee045d..bd272bec 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -1012,6 +1012,8 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe ? (float) $thold_data['rrd_step'] : 0.0; $sample_interval = max($rrd_step, (float) $poller_interval); + // Use a two-cycle floor so normal scheduler jitter does not discard the + // only usable pair even when an RRD heartbeat is tighter than poll cadence. $rrd_heartbeat = is_numeric($thold_data['rrd_heartbeat'] ?? null) && $thold_data['rrd_heartbeat'] > 0 ? max((float) $thold_data['rrd_heartbeat'], 2 * $sample_interval) : 2 * $sample_interval; @@ -1177,7 +1179,19 @@ function thold_calculate_expression($thold, $currentval, &$rrd_reindexed, &$rrd_ if (cacti_sizeof($source) && $source['data_source_type_id'] == 1) { $value = $current_sample; } elseif (cacti_sizeof($source)) { - $value = get_current_value($thold['local_data_id'], $dsname, 0, ''); + $value = ''; + + if (read_config_option('dsstats_enable') == 'on') { + $value = db_fetch_cell_prepared('SELECT calculated + FROM data_source_stats_hourly_last + WHERE local_data_id = ? + AND rrd_name = ?', + [$thold['local_data_id'], $dsname]); + } + + if (!is_numeric($value) || $value == -90909090909) { + $value = get_current_value($thold['local_data_id'], $dsname, 0, ''); + } } else { $value = ''; } @@ -1469,7 +1483,9 @@ function thold_calculate_percent($thold, $currentval, $rrd_reindexed) { // forced the percentage to zero and kept a low threshold alerting. $t = $rrd_reindexed[$thold['local_data_id']][$thold['percent_ds']]; - if (is_numeric($t) && $t != 0) { + if (!is_numeric($t)) { + $currentval = ''; + } elseif ($t != 0) { $currentval = ($currentval / $t) * 100; } else { $currentval = 0; From 8abca7940484469223d1c4267c93299252744584 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 18 Aug 2026 11:02:07 -0700 Subject: [PATCH 17/17] fix: deduplicate backward-clock warnings --- tests/Unit/TholdGetCurrentvalTest.php | 6 +++++ thold_functions.php | 36 +++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php index c52fe43f..7cfbaf5b 100644 --- a/tests/Unit/TholdGetCurrentvalTest.php +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -537,13 +537,19 @@ public function testOutOfOrderCounterAndDeriveSamplesAreUnknown(): void { ['lasttime' => 1700000400, 'oldvalue' => 100], thold_sample_persistence($thold, ['traffic_in' => 700], 1700000400) ); + + CactiStubs::reset(); $this->assertSame([ 'sample_row' => "(9, 1, '', FROM_UNIXTIME(1700000300), '700')", 'status_row' => null, ], thold_polling_sample_row($thold, ['traffic_in' => 700], '', 1700000300)); + $this->assertCount(1, CactiStubs::$log); + $this->assertStringContainsString('clock moved backwards', CactiStubs::$log[0]); CactiStubs::reset(); $this->assertTrue(thold_daemon_persist_sample($thold, ['traffic_in' => 700], '', 1700000300)); + $this->assertCount(1, CactiStubs::$log); + $this->assertStringContainsString('clock moved backwards', CactiStubs::$log[0]); $call = end(CactiStubs::$calls); $this->assertSame([1, '', 1700000300, 700, 9], $call['params']); diff --git a/thold_functions.php b/thold_functions.php index bd272bec..2bd53824 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -837,6 +837,28 @@ function thold_counter_wrap_delta($oldvalue, $newvalue) { return (4294967296 - $oldvalue) + $newvalue; } +/** + * Whether a valid current sample predates the stored sample clock. + * + * @param array $thold_data + * @param array $item + * @param int $currenttime + * + * @return bool + */ +function thold_sample_clock_moved_backward(array $thold_data, array $item, $currenttime) { + $name = (string) ($thold_data['name'] ?? ''); + $currenttime = (int) $currenttime; + $lasttime = (int) ($thold_data['lasttime'] ?? 0); + + return $name !== '' + && $currenttime > 0 + && $lasttime > 0 + && $currenttime < $lasttime + && isset($item[$name]) + && is_numeric($item[$name]); +} + /** * Persist a raw sample and its timestamp as one causal pair. * @@ -856,7 +878,7 @@ function thold_sample_persistence(array $thold_data, array $item, $currenttime) $lasttime = (int) ($thold_data['lasttime'] ?? 0); if ($name !== '' && $currenttime > 0 && $currenttime !== $lasttime && isset($item[$name]) && is_numeric($item[$name])) { - if ($lasttime > 0 && $currenttime < $lasttime) { + if (thold_sample_clock_moved_backward($thold_data, $item, $currenttime)) { cacti_log(sprintf( 'WARNING: Threshold %s sample clock moved backwards; re-anchoring its value and timestamp.', $thold_data['id'] ?? ($thold_data['thold_id'] ?? 'unknown') @@ -910,10 +932,12 @@ function thold_daemon_persist_sample(array $thold_data, array $item, $currentval return false; } - thold_log_unavailable_transition($thold_data, $currentval); - $sample = thold_sample_persistence($thold_data, $item, $currenttime); + if (!thold_sample_clock_moved_backward($thold_data, $item, $currenttime)) { + thold_log_unavailable_transition($thold_data, $currentval); + } + if ($sample['lasttime'] <= 0) { return db_execute_prepared('UPDATE thold_data SET tcheck = ?, lastread = ? @@ -946,10 +970,12 @@ function thold_polling_sample_row(array $thold_data, array $item, $currentval, $ return ['sample_row' => null, 'status_row' => null]; } - thold_log_unavailable_transition($thold_data, $currentval); - $sample = thold_sample_persistence($thold_data, $item, $currenttime); + if (!thold_sample_clock_moved_backward($thold_data, $item, $currenttime)) { + thold_log_unavailable_transition($thold_data, $currentval); + } + if ($sample['lasttime'] <= 0) { return [ 'sample_row' => null,