From 5661a0d6723763f38a172bac5c4731b7d2738f31 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 23:53:21 -0700 Subject: [PATCH 1/6] fix(poller): evaluate the right thresholds and clear only what was checked Three defects in includes/polling.php. The poller_id alternation had no parentheses, and AND binds tighter than OR, so the clause read as (... AND h.poller_id = 1) OR (h.poller_id IS NULL AND tcheck = 1 AND status = 3) The first branch carries neither the dirty-flag nor the device-status filter, so on poller 1 every enabled threshold was re-evaluated on every run against whatever lastread happened to be there, including for devices that were down. The second branch cannot match, since the LEFT JOIN gives a NULL host a NULL status. Confirmed against MariaDB on seeded rows: the clause returned a threshold with no new data and one on a down device; parenthesised, it returns neither. The dirty flag was then cleared by poller, or across the whole table, rather than for the identifiers just evaluated. Any reading arriving between the select and that update was discarded unchecked. It now clears exactly what was visited, which also collapses three statements into one; the select already scopes to this poller, so the identifiers carry that scoping with them. array_chunk() takes the size of each chunk, not how many to make, so 500 readings became 50 chunks of 10 rather than 10 of 50, and each chunk costs a round trip. Refs #783 Signed-off-by: Thomas Vincent --- composer.json | 49 +++++++ includes/polling.php | 47 +++--- tests/Unit/PollerSchedulingTest.php | 212 ++++++++++++++++++++++++++++ tests/docker/Dockerfile | 29 ++++ tests/docker/docker-compose.yml | 15 ++ 5 files changed, 326 insertions(+), 26 deletions(-) create mode 100644 composer.json create mode 100644 tests/Unit/PollerSchedulingTest.php create mode 100644 tests/docker/Dockerfile create mode 100644 tests/docker/docker-compose.yml diff --git a/composer.json b/composer.json new file mode 100644 index 00000000..94aead2f --- /dev/null +++ b/composer.json @@ -0,0 +1,49 @@ +{ + "_comment": [ + "+-------------------------------------------------------------------------+", + "| Copyright (C) 2004-2026 The Cacti Group |", + "| |", + "| This program is free software; you can redistribute it and/or |", + "| modify it under the terms of the GNU General Public License |", + "| as published by the Free Software Foundation; either version 2 |", + "| of the License, or (at your option) any later version. |", + "| |", + "| This program is distributed in the hope that it will be useful, |", + "| but WITHOUT ANY WARRANTY; without even the implied warranty of |", + "| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |", + "| GNU General Public License for more details. |", + "+-------------------------------------------------------------------------+", + "| Cacti: The Complete RRDtool-based Graphing Solution |", + "+-------------------------------------------------------------------------+", + "| http://www.cacti.net/ |", + "+-------------------------------------------------------------------------+" + ], + "name": "cacti/plugin-thold", + "description": "Thold Plugin for Cacti", + "type": "project", + "license": "GPL-2.0-only", + "require-dev": { + "overtrue/phplint": "^9.6", + "phpunit/phpunit": "^10.5.64" + }, + "scripts": { + "lint": "phplint --no-cache --exclude=vendor ", + "test": "phpunit --display-warnings", + "test:coverage": "phpunit --display-warnings --coverage-clover=coverage/clover.xml", + "test:docker": "docker compose -f tests/docker/docker-compose.yml run --rm phpunit" + }, + "config": { + "sort-packages": true, + "vendor-dir": "vendor", + "platform": { + "php": "8.1.0" + }, + "platform-check": true + }, + "autoload-dev": { + "classmap": [ + "tests/Helpers/", + "tests/TestCase.php" + ] + } +} diff --git a/includes/polling.php b/includes/polling.php index c5c2c1d6..0e0ca5a1 100644 --- a/includes/polling.php +++ b/includes/polling.php @@ -75,13 +75,8 @@ function thold_poller_output(&$rrd_update_array) { if ($local_data_ids != '') { if (read_config_option('thold_daemon_enable') == 'on') { - $chunks = ceil(sizeof($rrd_update_array) / 50); - - if ($chunks < 1) { - $chunks = 1; - } - - $rrd_update_array_chunks = array_chunk($rrd_update_array, $chunks, true); + // array_chunk() takes the size of each chunk, not how many to make. + $rrd_update_array_chunks = array_chunk($rrd_update_array, 50, true); foreach ($rrd_update_array_chunks as $rrd_update_array_chunk) { $rrd_reindexed = []; @@ -271,7 +266,7 @@ function thold_check_all_thresholds() { ) ) ) - AND h.poller_id = 1 OR h.poller_id IS NULL + AND (h.poller_id = 1 OR h.poller_id IS NULL) AND td.tcheck = 1 AND h.status = 3"; } else { @@ -322,28 +317,28 @@ function thold_check_all_thresholds() { $total_tholds = sizeof($tholds); + $checked = []; + foreach ($tholds as $thold) { thold_check_threshold($thold); + + $checked[] = (int) $thold['id']; } - if (read_config_option('remote_storage_method') == 1) { - if ($config['poller_id'] == 1) { - db_execute('UPDATE thold_data AS td - LEFT JOIN host AS h - ON td.host_id = h.id - SET tcheck = 0 - WHERE h.poller_id = 1 - OR h.poller_id IS NULL'); - } else { - db_execute_prepared('UPDATE thold_data AS td - INNER JOIN host AS h - ON td.host_id = h.id - SET td.tcheck = 0 - WHERE h.poller_id = ?', - [$config['poller_id']]); - } - } else { - db_execute('UPDATE thold_data AS td SET td.tcheck = 0'); + /* + * Clear the dirty flag on what was evaluated, and nothing else. Clearing + * by poller, or across the whole table, also cleared the thresholds whose + * data arrived after the select above, so those readings were dropped + * without ever being checked. The select already scopes to this poller, + * so the identifiers carry that scoping with them. + */ + if (cacti_sizeof($checked)) { + $placeholders = implode(', ', array_fill(0, cacti_sizeof($checked), '?')); + + db_execute_prepared('UPDATE thold_data + SET tcheck = 0 + WHERE id IN (' . $placeholders . ')', + $checked); } return $total_tholds; diff --git a/tests/Unit/PollerSchedulingTest.php b/tests/Unit/PollerSchedulingTest.php new file mode 100644 index 00000000..bf3f7270 --- /dev/null +++ b/tests/Unit/PollerSchedulingTest.php @@ -0,0 +1,212 @@ + 2, + 'description' => 'core-switch-1', + 'hostname' => '10.0.0.1', + 'location' => 'rack 4', + 'site_id' => 1, + 'status' => 3, + 'status_fail_date' => '2026-01-01 00:00:00', + 'status_rec_date' => '2026-01-02 00:00:00', + 'status_last_error' => '', + 'snmp_engine_id' => '', + 'notes' => '', + ]); + } + + /** + * A threshold row with every column the evaluator reads, set so that it + * evaluates quietly. Only the identifier varies between rows here; what is + * under test is which rows are visited, not what visiting them decides. + * + * @param int $id + * + * @return array + */ + private function threshold($id) { + return [ + 'id' => $id, 'host_id' => 2, 'local_data_id' => 4, 'local_graph_id' => 9, + 'data_template_rrd_id' => 3, 'name' => 'CPU', 'name_cache' => 'CPU', + 'data_source_name' => 'traffic_in', 'thold_type' => 0, 'data_type' => 0, + 'lastread' => 50, 'oldvalue' => 50, 'lasttime' => 0, 'rrd_step' => 300, + 'thold_hi' => '', 'thold_low' => '', 'thold_warning_hi' => '', 'thold_warning_low' => '', + 'thold_fail_trigger' => 1, 'thold_warning_fail_trigger' => 1, + 'thold_fail_count' => 0, 'thold_warning_fail_count' => 0, + 'thold_alert' => 0, 'repeat_alert' => 0, + 'time_hi' => '', 'time_low' => '', 'time_warning_hi' => '', 'time_warning_low' => '', + 'time_fail_trigger' => 1, 'time_warning_fail_trigger' => 1, + 'time_fail_length' => 300, 'time_warning_fail_length' => 300, + 'bl_fail_count' => 0, 'bl_alert' => 0, 'bl_pct_down' => '', 'bl_pct_up' => '', + 'bl_fail_trigger' => 1, 'bl_ref_time_range' => 3600, 'bl_type' => 0, + 'bl_cf' => 'AVG', 'bl_thold_valid' => 0, 'cdef' => 0, + 'notify_warning' => 0, 'notify_alert' => 0, 'notify_extra' => '', + 'notify_warning_extra' => '', 'persist_ack' => '', 'reset_ack' => '', + 'acknowledgment' => '', 'exempt' => '', 'restored_alert' => '', + 'syslog_enabled' => '', 'syslog_priority' => 5, 'syslog_facility' => 1, + 'snmp_event_severity' => 3, 'snmp_event_description' => '', 'snmp_engine_id' => '', + 'trigger_cmd_high' => '', 'trigger_cmd_low' => '', 'trigger_cmd_norm' => '', + 'notes' => '', 'dnotes' => '', 'external_id' => '', + 'email_subject' => '', 'email_subject_warn' => '', 'email_subject_restoral' => '', + 'graph_timespan' => 7, 'show_units' => '', 'units_suffix' => '', 'decimals' => 2, + 'format_file' => '', 'thold_enabled' => 'on', 'thold_daemon_id' => 0, + ]; + } + + /** + * Only the thresholds that were evaluated get their dirty flag cleared. + * + * Clearing by poller, or across the whole table, also cleared the ones + * whose data arrived after the select, so those readings were dropped + * without ever being checked. + * + * @return void + */ + public function testOnlyTheEvaluatedThresholdsAreCleared(): void { + CactiStubs::willReturnFor('db_fetch_assoc', 'FROM thold_data', [ + $this->threshold(7), + $this->threshold(9), + ]); + + thold_check_all_thresholds(); + + $clears = array_values(array_filter(CactiStubs::callsTo('db_execute_prepared'), static function ($call) { + return strpos($call['sql'], 'SET tcheck = 0') !== false; + })); + + $this->assertCount(1, $clears); + $this->assertSame([7, 9], $clears[0]['params']); + $this->assertStringContainsString('WHERE id IN (?, ?)', $clears[0]['sql']); + } + + /** + * @return void + */ + public function testNothingIsClearedWhenNothingWasEvaluated(): void { + CactiStubs::willReturnFor('db_fetch_assoc', 'FROM thold_data', []); + + thold_check_all_thresholds(); + + $clears = array_filter(CactiStubs::$calls, static function ($call) { + return strpos($call['sql'], 'tcheck = 0') !== false; + }); + + $this->assertSame([], array_values($clears)); + } + + /** + * @return void + */ + public function testTheRunReportsHowManyThresholdsItEvaluated(): void { + CactiStubs::willReturnFor('db_fetch_assoc', 'FROM thold_data', [ + $this->threshold(1), + $this->threshold(2), + $this->threshold(3), + ]); + + $this->assertSame(3, thold_check_all_thresholds()); + } + + /** + * The whole table is never cleared in one statement. + * + * @return void + */ + public function testTheDirtyFlagIsNeverClearedTableWide(): void { + CactiStubs::willReturnFor('db_fetch_assoc', 'FROM thold_data', [ + $this->threshold(7), + ]); + + thold_check_all_thresholds(); + + foreach (CactiStubs::$calls as $call) { + if (strpos($call['sql'], 'tcheck = 0') !== false) { + $this->assertStringContainsString('WHERE id IN', $call['sql']); + } + } + } + + /** + * A structural guard rather than a behavioural one: the defect is SQL + * operator precedence, and proving it needs a database to run the query + * against. Without the parentheses, AND binds tighter than OR and the + * clause reads as + * + * (... AND h.poller_id = 1) OR (h.poller_id IS NULL AND tcheck = 1 AND status = 3) + * + * whose first branch carries neither the dirty-flag nor the device-status + * filter, so every enabled threshold is re-evaluated on every run. + * + * @return void + */ + public function testThePollerAlternationIsParenthesised(): void { + $src = file_get_contents(dirname(__DIR__, 2) . '/includes/polling.php'); + + $this->assertStringContainsString('AND (h.poller_id = 1 OR h.poller_id IS NULL)', $src); + $this->assertStringNotContainsString("AND h.poller_id = 1 OR h.poller_id IS NULL\n", $src); + } + + /** + * array_chunk() takes the size of each chunk. Passing a count meant 500 + * readings became 50 chunks of 10 rather than 10 chunks of 50, and each + * chunk costs a round trip. + * + * @return void + */ + public function testTheDaemonBatchesReadingsFiftyAtATime(): void { + $src = file_get_contents(dirname(__DIR__, 2) . '/includes/polling.php'); + + $this->assertStringContainsString('array_chunk($rrd_update_array, 50, true)', $src); + $this->assertStringNotContainsString('ceil(sizeof($rrd_update_array) / 50)', $src); + } +} diff --git a/tests/docker/Dockerfile b/tests/docker/Dockerfile new file mode 100644 index 00000000..518f7321 --- /dev/null +++ b/tests/docker/Dockerfile @@ -0,0 +1,29 @@ +# Test runner for the Thold plugin. +# +# Pinned to PHP 8.1 because that is the oldest interpreter the CI matrix +# covers; what passes here passes on 8.2-8.4. pcov rather than Xdebug: line +# coverage is the only debug feature the suite needs and pcov is far cheaper. +FROM php:8.1-cli-alpine@sha256:7949370448b0b4d9787776dc5968e0fd8d48763292344b5fbf21539441228a98 + +# git is needed by the changed-line coverage gate, which diffs against the +# base branch. +RUN apk add --no-cache git gmp-dev \ + && docker-php-ext-install gmp \ + && apk add --no-cache --virtual .build-deps $PHPIZE_DEPS \ + && pecl install pcov \ + && docker-php-ext-enable pcov \ + && apk del .build-deps + +COPY --from=composer:2@sha256:4d71c3c2109c61d5415544264b59ad4087e4c5b7244481723664138fd36d5040 /usr/bin/composer /usr/bin/composer + +# The plugin lives where Cacti would put it, because thold_functions.php +# resolves its own includes through $config['base_path'] . '/plugins/thold'. +# No network or database is involved; the Cacti framework functions themselves +# are stubbed in tests/bootstrap.php. +WORKDIR /cacti/plugins/thold + +ENV COMPOSER_ALLOW_SUPERUSER=1 \ + COMPOSER_NO_INTERACTION=1 \ + COMPOSER_CACHE_DIR=/tmp/composer-cache + +CMD ["sh", "-c", "composer install --no-progress --no-ansi && composer test"] diff --git a/tests/docker/docker-compose.yml b/tests/docker/docker-compose.yml new file mode 100644 index 00000000..99d38b47 --- /dev/null +++ b/tests/docker/docker-compose.yml @@ -0,0 +1,15 @@ +# Local mirror of the unit-test CI job. `docker compose -f +# tests/docker/docker-compose.yml run --rm phpunit` runs exactly what CI runs. +services: + phpunit: + build: + context: . + dockerfile: Dockerfile + image: cacti-thold-test:php8.1 + working_dir: /cacti/plugins/thold + volumes: + - ../..:/cacti/plugins/thold + - composer-cache:/tmp/composer-cache + +volumes: + composer-cache: From ccfe70dd762cde31dd1ad1a3491bf148b1ca1b05 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 23:57:26 -0700 Subject: [PATCH 2/6] test: cover the daemon batching path The batch size only takes effect when the daemon is enabled, which none of the other cases exercise, so the changed line went uncovered. The stub can now answer a matched query with a callable, which lets the threshold lookup return just the batch it was asked about. Signed-off-by: Thomas Vincent --- tests/Unit/PollerSchedulingTest.php | 38 +++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/Unit/PollerSchedulingTest.php b/tests/Unit/PollerSchedulingTest.php index bf3f7270..b9f2a8ae 100644 --- a/tests/Unit/PollerSchedulingTest.php +++ b/tests/Unit/PollerSchedulingTest.php @@ -176,6 +176,44 @@ public function testTheDirtyFlagIsNeverClearedTableWide(): void { } } + /** + * The daemon path batches readings before writing them, and the batch size + * is what array_chunk() takes. Passing a count meant 500 readings became + * 50 batches of 10 rather than 10 of 50, and each batch costs a round trip. + * + * @return void + */ + public function testTheDaemonWritesReadingsInBatchesOfFifty(): void { + CactiStubs::$configOptions['thold_daemon_enable'] = 'on'; + + $readings = []; + + for ($i = 1; $i <= 120; $i++) { + $readings[] = ['local_data_id' => $i, 'times' => [1700000000 => ['traffic_in' => 5]]]; + } + + // One threshold per reading, answering with just the batch that was asked + // about so each write matches the readings it was given. + CactiStubs::willReturnFor('db_fetch_assoc', 'SELECT id, local_data_id, thread_id', static function ($sql) { + preg_match('/IN \(([^)]*)\)/', $sql, $matches); + + return array_map(static function ($id) { + $id = (int) trim($id); + + return ['id' => $id, 'local_data_id' => $id, 'thread_id' => 1]; + }, explode(',', $matches[1])); + }); + + thold_poller_output($readings); + + $inserts = array_values(array_filter(CactiStubs::$calls, static function ($call) { + return strpos($call['sql'], 'plugin_thold_daemon_data') !== false; + })); + + // 120 readings at 50 to a batch is three writes, not one per reading. + $this->assertCount(3, $inserts); + } + /** * A structural guard rather than a behavioural one: the defect is SQL * operator precedence, and proving it needs a database to run the query From f295a73dbc24922efb91262b1c8c3168d8425097 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:16:12 -0700 Subject: [PATCH 3/6] chore: remove plugin-local Composer manifest --- composer.json | 49 ------------------------------------------------- 1 file changed, 49 deletions(-) delete mode 100644 composer.json diff --git a/composer.json b/composer.json deleted file mode 100644 index 94aead2f..00000000 --- a/composer.json +++ /dev/null @@ -1,49 +0,0 @@ -{ - "_comment": [ - "+-------------------------------------------------------------------------+", - "| Copyright (C) 2004-2026 The Cacti Group |", - "| |", - "| This program is free software; you can redistribute it and/or |", - "| modify it under the terms of the GNU General Public License |", - "| as published by the Free Software Foundation; either version 2 |", - "| of the License, or (at your option) any later version. |", - "| |", - "| This program is distributed in the hope that it will be useful, |", - "| but WITHOUT ANY WARRANTY; without even the implied warranty of |", - "| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |", - "| GNU General Public License for more details. |", - "+-------------------------------------------------------------------------+", - "| Cacti: The Complete RRDtool-based Graphing Solution |", - "+-------------------------------------------------------------------------+", - "| http://www.cacti.net/ |", - "+-------------------------------------------------------------------------+" - ], - "name": "cacti/plugin-thold", - "description": "Thold Plugin for Cacti", - "type": "project", - "license": "GPL-2.0-only", - "require-dev": { - "overtrue/phplint": "^9.6", - "phpunit/phpunit": "^10.5.64" - }, - "scripts": { - "lint": "phplint --no-cache --exclude=vendor ", - "test": "phpunit --display-warnings", - "test:coverage": "phpunit --display-warnings --coverage-clover=coverage/clover.xml", - "test:docker": "docker compose -f tests/docker/docker-compose.yml run --rm phpunit" - }, - "config": { - "sort-packages": true, - "vendor-dir": "vendor", - "platform": { - "php": "8.1.0" - }, - "platform-check": true - }, - "autoload-dev": { - "classmap": [ - "tests/Helpers/", - "tests/TestCase.php" - ] - } -} From d8f570209dfc151ce334be2dfbfbba7169964521 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:17:20 -0700 Subject: [PATCH 4/6] test: isolate poller scheduling Cacti time stub --- tests/Unit/PollerSchedulingTest.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/Unit/PollerSchedulingTest.php b/tests/Unit/PollerSchedulingTest.php index b9f2a8ae..9a3b7be4 100644 --- a/tests/Unit/PollerSchedulingTest.php +++ b/tests/Unit/PollerSchedulingTest.php @@ -34,6 +34,17 @@ protected function setUp(): void { parent::setUp(); $GLOBALS['config']['poller_id'] = 1; + $GLOBALS['config']['base_path'] = sys_get_temp_dir() . '/thold-poller-scheduling'; + + $plugins = $GLOBALS['config']['base_path'] . '/plugins'; + + if (!is_dir($plugins)) { + mkdir($plugins, 0777, true); + } + + if (!file_exists($plugins . '/thold')) { + symlink(dirname(__DIR__, 2), $plugins . '/thold'); + } /* * includes/polling.php includes Cacti's lib/time.php at call time. From e31ec3b76650fc04b23b576f632d46560cc73ab1 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:47:13 -0700 Subject: [PATCH 5/6] ci: keep plugin PR integration checks on pinned Cacti --- .github/workflows/plugin-ci-workflow.yml | 9 --------- 1 file changed, 9 deletions(-) diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 5e4f3db6..79b16b7b 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -35,21 +35,12 @@ jobs: integration-test: runs-on: ${{ matrix.os }} - # A failure against the pinned release is a real failure. The develop entry - # is advisory: it is how a core regression becomes visible here, but it must - # not turn the plugin's own pull requests red. - continue-on-error: ${{ matrix.cacti != 'release/1.2.31' }} - strategy: fail-fast: false matrix: php: ['8.1', '8.2', '8.3', '8.4'] os: [ubuntu-latest] cacti: ['release/1.2.31'] - include: - - php: '8.4' - os: ubuntu-latest - cacti: 'develop' services: mariadb: From 437a10df6d42d08553b749b4746518f8b36427d5 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:59:50 -0700 Subject: [PATCH 6/6] ci: bound package index refreshes --- .github/workflows/plugin-ci-workflow.yml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 79b16b7b..e17554e8 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -86,7 +86,24 @@ jobs: echo "PHP_BINARY=$(command -v php)" >> "$GITHUB_ENV" - name: Run apt-get update - run: sudo apt-get update + run: | + for attempt in 1 2 3; do + if sudo timeout 3m apt-get \ + -o Dpkg::Lock::Timeout=60 \ + -o Acquire::Retries=3 \ + -o Acquire::http::Timeout=30 \ + -o Acquire::https::Timeout=30 \ + update; then + exit 0 + fi + + if [ "$attempt" -lt 3 ]; then + sleep 10 + fi + done + + echo 'apt-get update failed after three bounded attempts.' >&2 + exit 1 - name: Install System Dependencies run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping