From 17ad98bb2078b14d6bf6b563e15282e9e47d87c6 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 Signed-off-by: Thomas Vincent --- 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 ae48cd43..8ab69e1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,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 e174214e..d25ea248 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -837,6 +837,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 a334c5796e8f0c47d03ccd23879f195e4aaf94ec 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 Signed-off-by: Thomas Vincent --- 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 d25ea248..567a77cf 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -840,6 +840,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 @@ -847,18 +850,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 25d9faf99d1b55c8e6378afaefca1f13f35dce4b 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 Signed-off-by: Thomas Vincent --- 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 567a77cf..0e5c5b9f 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -890,6 +890,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 5e9f771b2e757a0ec6a91f88a5592bd58c4ba8fd 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 Signed-off-by: Thomas Vincent --- 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 0e5c5b9f..52910295 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -891,29 +891,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) { @@ -934,6 +935,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']])) { @@ -943,7 +947,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']; @@ -980,7 +984,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; @@ -991,7 +995,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 a82f6257f19e3cc6a452c11c5fa641476715534f 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 Signed-off-by: Thomas Vincent --- 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 52910295..9483915d 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -935,7 +935,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 0904c6e0a00689a8b70190b92ef021050c02d984 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 Signed-off-by: Thomas Vincent --- 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 9483915d..4905b4e7 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -935,8 +935,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 = ''; @@ -981,23 +993,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 dcef42ea6659f9581dc45091d3657074cd1936c0 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 Signed-off-by: Thomas Vincent --- 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 4905b4e7..a3fd590f 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -874,20 +874,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]); } /** @@ -901,25 +907,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)) { @@ -932,13 +968,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']; @@ -993,7 +1023,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 122d2d0d28955f3104158a8bd61fcdc695719a97 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 Signed-off-by: Thomas Vincent --- 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 568c1860762fd7091e4ba93ac1572eef4f6bf6ba 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 Signed-off-by: Thomas Vincent --- 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 a3fd590f..ebf6c034 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -853,12 +853,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, ]; } @@ -998,7 +1000,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 = ?', @@ -1023,12 +1025,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 412a2c94b17d9557b5efdd001a588393193d5499 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 Signed-off-by: Thomas Vincent --- 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 ebf6c034..ffd45daf 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -855,7 +855,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]]; } @@ -1052,7 +1052,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 @@ -1420,9 +1422,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 be2bce53ce87e9d615ed4c959bce182a321cae65 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 Signed-off-by: Thomas Vincent --- 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 ffd45daf..9237f5ba 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -943,7 +943,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()); @@ -969,18 +969,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 4d012fdb765c604360093fd23359145aa4ee33a6 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 Signed-off-by: Thomas Vincent --- 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 9237f5ba..72ec1a14 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -876,7 +876,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; @@ -886,16 +887,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]); } /** @@ -909,7 +910,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]; @@ -920,12 +922,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, ]; @@ -979,10 +981,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 = ''; @@ -1041,7 +1046,7 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe $currentval = $currentval / $step; } } else { - $currentval = $thold_data['lasttime'] > 0 ? '' : 0; + $currentval = ''; } break; @@ -1049,7 +1054,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 fae2d378b440d45ae048c68b28815d8d57323155 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 Signed-off-by: Thomas Vincent --- 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 72ec1a14..d573d3f1 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -982,13 +982,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']])) { @@ -1117,7 +1125,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 @@ -1133,6 +1141,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 @@ -1150,11 +1162,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); @@ -4832,8 +4844,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 e5431d0f51b4fc4a2d5c6d4463a58b948ffcf318 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 Signed-off-by: Thomas Vincent --- 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 d573d3f1..9a5611b9 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -1135,32 +1135,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 04d2417eede36bb613469490dec576467f82a0ce 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 Signed-off-by: Thomas Vincent --- 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 | 103 +++++++++++++++--- 9 files changed, 227 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ab69e1b..daa563c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,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 d23613f5..eaac49a0 100644 --- a/tests/Unit/ThresholdHiLowCharacterizationTest.php +++ b/tests/Unit/ThresholdHiLowCharacterizationTest.php @@ -255,9 +255,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 9a5611b9..902d9f74 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -855,7 +855,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]]; } @@ -865,6 +872,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. * @@ -877,12 +904,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) { @@ -907,22 +936,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], ]; } @@ -1041,9 +1072,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) { @@ -1136,6 +1165,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 ''; } @@ -1426,17 +1488,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) { @@ -2507,6 +2569,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'); @@ -5088,7 +5155,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); @@ -5117,7 +5184,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 @@ -5127,7 +5194,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 61bcdcf453de2db8d2280dc8aaede0dee286bbb4 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 Signed-off-by: Thomas Vincent --- 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 902d9f74..b6f4cedd 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 791860cd9ed907c2104c9effdae8215afc6fb243 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 Signed-off-by: Thomas Vincent --- 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 b6f4cedd..a6629815 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,