fix(thold): correct counter delta, wrap modulus and percent denominator (#791)

* test: add the PHP 8.1 unit-test harness

Same harness as #773 and #788, with gmp added to the image so the 64-bit
counter arithmetic can be tested exactly.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* fix(thold): correct the counter delta and the percent denominator

A previous counter reading of exactly zero was treated as no reading at all,
so the first interval after a device reboot reported a rate of zero. The wrap
modulus was 2^32-1 and 2^64-1 rather than 2^32 and 2^64, losing one count per
wrap, and the 64-bit literal exceeded PHP_INT_MAX so it was parsed as a float
and lost about eleven bits before the subtraction.

The percent-of denominator was cast to int, so a denominator below one
truncated to zero and forced the result to zero, keeping any configured low
threshold in permanent breach.

Refs #785

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* fix(daemon): store the previous reading in oldvalue, not a timestamp

When a data source produced no sample this cycle the daemon wrote
$currenttime - $rrd_step into oldvalue, so the next poll computed a delta
against a Unix timestamp, took the overflow branch and fabricated a rate in
the billions. The non-daemon path already carries the previous oldvalue
forward; this matches it.

Refs #785

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* test: follow Cacti's test layout and composer scripts

Adopts the conventions from Cacti core: tests/bootstrap-unit.php, tests/Helpers
for the stubs, a phpunit.xml carrying error_reporting -1 and
CACTI_TEST_BOOTSTRAP, and composer lint / test / test:coverage scripts so CI
runs the same commands a developer does. The dev toolchain is Cacti's, pinned
to the same platform php 8.1.0.

Cacti core runs Pest and this suite does not, because pest ^2 does not resolve
on PHP 8.1 -- the platform Cacti's own composer.json pins. Releases up to
v2.36.0 conflict with phpunit 10.5.62 and later, every earlier 10.x release is
blocked by advisory PKSA-z3gr-8qht-p93v, and v2.36.1, which does resolve,
requires PHP 8.2. The stack installs on 8.2 and above; 8.1 is the floor this
plugin's CI matrix targets. The tests are written in the plain PHPUnit class
style that Cacti's tests/Pest.php explicitly supports, so they run unchanged
under Pest wherever it is installable.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* ci: keep plugin PR integration checks on pinned Cacti

* fix(counter): handle non-integer 64-bit readings safely

* ci: bound package index refreshes

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

.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:
@@ -95,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

tests/Unit/TholdCalculatePercentTest.php

@@ -0,0 +1,100 @@
+<?php
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004-2026 The Cacti Group                                 |
+ |                                                                         |
+ | This program is free software; you can redistribute it and/or           |
+ | modify it under the terms of the GNU General Public License             |
+ | as published by the Free Software Foundation; either version 2          |
+ | of the License, or (at your option) any later version.                  |
+ +-------------------------------------------------------------------------+
+ | Cacti: The Complete RRDtool-based Graphing Solution                     |
+ +-------------------------------------------------------------------------+
+ | http://www.cacti.net/                                                   |
+ +-------------------------------------------------------------------------+
+*/
+
+/**
+ * thold_calculate_percent() expresses a reading as a percentage of a second
+ * data source on the same RRD.
+ *
+ * An empty string is the engine's "no usable value" sentinel; zero is a real
+ * percentage and is compared against the bounds as one.
+ */
+final class TholdCalculatePercentTest extends TestCase {
+	/**
+	 * @return void
+	 */
+	public static function setUpBeforeClass(): void {
+		self::loadPluginSource('thold_functions.php');
+	}
+
+	/**
+	 * @return array<string, mixed>
+	 */
+	private function threshold() {
+		return ['percent_ds' => 'total', 'local_data_id' => 4];
+	}
+
+	/**
+	 * @param float|int|string $denominator
+	 * @param float|int|string $reading
+	 *
+	 * @return mixed
+	 */
+	private function percent($denominator, $reading = 50) {
+		return thold_calculate_percent($this->threshold(), $reading, [4 => ['total' => $denominator]]);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testReadingIsExpressedAsAPercentageOfTheReference(): void {
+		$this->assertEqualsWithDelta(25, $this->percent(200), 1.0e-9);
+	}
+
+	/**
+	 * A denominator below one used to truncate to zero, forcing the result to
+	 * zero and keeping any configured low threshold in permanent breach.
+	 *
+	 * @return void
+	 */
+	public function testFractionalDenominatorIsNotTruncated(): void {
+		$this->assertEqualsWithDelta(1000, $this->percent(0.5, 5), 1.0e-9);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testNegativeDenominatorGivesANegativePercentage(): void {
+		$this->assertEqualsWithDelta(-25, $this->percent(-200), 1.0e-9);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testZeroDenominatorGivesZeroRatherThanDividingByZero(): void {
+		$this->assertSame(0, $this->percent(0));
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testNonNumericDenominatorGivesZero(): void {
+		$this->assertSame(0, $this->percent('U'));
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testNonNumericReadingYieldsTheNoValueSentinel(): void {
+		$this->assertSame('', $this->percent(200, 'U'));
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testMissingReferenceDataSourceYieldsTheNoValueSentinel(): void {
+		$this->assertSame('', thold_calculate_percent($this->threshold(), 50, [4 => ['other' => 200]]));
+	}
+}

tests/Unit/TholdGetCurrentvalTest.php

@@ -0,0 +1,176 @@
+<?php
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004-2026 The Cacti Group                                 |
+ |                                                                         |
+ | This program is free software; you can redistribute it and/or           |
+ | modify it under the terms of the GNU General Public License             |
+ | as published by the Free Software Foundation; either version 2          |
+ | of the License, or (at your option) any later version.                  |
+ +-------------------------------------------------------------------------+
+ | Cacti: The Complete RRDtool-based Graphing Solution                     |
+ +-------------------------------------------------------------------------+
+ | http://www.cacti.net/                                                   |
+ +-------------------------------------------------------------------------+
+*/
+
+/**
+ * thold_get_currentval() turns a raw sample into the rate a threshold is
+ * compared against, per the data source type.
+ */
+final class TholdGetCurrentvalTest extends TestCase {
+	const GAUGE    = 1;
+	const COUNTER  = 2;
+	const DERIVE   = 3;
+	const ABSOLUTE = 4;
+
+	/**
+	 * @return void
+	 */
+	public static function setUpBeforeClass(): void {
+		self::loadPluginSource('thold_functions.php');
+	}
+
+	/**
+	 * @param array<string, mixed> $overrides
+	 *
+	 * @return array<string, mixed>
+	 */
+	private function threshold(array $overrides = []) {
+		return $overrides + [
+			'local_data_id'        => 4,
+			'name'                 => 'traffic_in',
+			'data_source_type_id'  => self::COUNTER,
+			'rrd_step'             => 300,
+			'rrd_maximum'          => 0,
+			'lasttime'             => 0,
+			'oldvalue'             => 100,
+		];
+	}
+
+	/**
+	 * @param array<string, mixed> $thold
+	 * @param float|int|string     $reading
+	 *
+	 * @return mixed
+	 */
+	private function currentValue(array $thold, $reading) {
+		$reindexed      = [4 => ['traffic_in' => $reading]];
+		$time_reindexed = [4 => 1700000300];
+		$item           = [];
+		$currenttime    = 0;
+
+		return thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testGaugeReturnsTheReadingUnchanged(): void {
+		$thold = $this->threshold(['data_source_type_id' => self::GAUGE]);
+
+		$this->assertSame(42, $this->currentValue($thold, 42));
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testAbsoluteDividesTheReadingByTheStep(): void {
+		$thold = $this->threshold(['data_source_type_id' => self::ABSOLUTE]);
+
+		$this->assertEqualsWithDelta(2, $this->currentValue($thold, 600), 1.0e-9);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testCounterReturnsTheDeltaOverTheStep(): void {
+		$thold = $this->threshold(['oldvalue' => 100]);
+
+		$this->assertEqualsWithDelta(2, $this->currentValue($thold, 700), 1.0e-9);
+	}
+
+	/**
+	 * A counter that legitimately read zero last cycle is not the same as
+	 * having no previous reading. Treating it as absent reports a rate of zero
+	 * for the first interval after a device reboot.
+	 *
+	 * @return void
+	 */
+	public function testCounterTreatsAPreviousReadingOfZeroAsReal(): void {
+		$thold = $this->threshold(['oldvalue' => 0]);
+
+		$this->assertEqualsWithDelta(2, $this->currentValue($thold, 600), 1.0e-9);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testCounterWithNoPreviousReadingYieldsZero(): void {
+		$thold = $this->threshold(['oldvalue' => '']);
+
+		$this->assertSame(0, $this->currentValue($thold, 600));
+	}
+
+	/**
+	 * A 32-bit counter that wraps has advanced by (2^32 - old) + new. Using
+	 * 2^32-1 as the modulus loses exactly one count per wrap.
+	 *
+	 * @return void
+	 */
+	public function testThirtyTwoBitWrapUsesTheCorrectModulus(): void {
+		$thold = $this->threshold(['oldvalue' => 4294967290, 'rrd_step' => 1]);
+
+		$this->assertEqualsWithDelta(11, $this->currentValue($thold, 5), 1.0e-9);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testSixtyFourBitWrapUsesTheCorrectModulus(): void {
+		$thold = $this->threshold(['oldvalue' => '18446744073709551610', 'rrd_step' => 1]);
+
+		$this->assertEqualsWithDelta(11, $this->currentValue($thold, 5), 1.0e-9);
+	}
+
+	/**
+	 * RRD values can arrive in scientific notation. GMP accepts only integer
+	 * strings, so these values must use the non-fatal floating-point fallback.
+	 *
+	 * @return void
+	 */
+	public function testSixtyFourBitWrapAcceptsScientificNotation(): void {
+		$thold = $this->threshold(['oldvalue' => '1.8446744073709552E+19', 'rrd_step' => 1]);
+
+		$this->assertEqualsWithDelta(5, $this->currentValue($thold, 5), 1.0e-9);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testDeriveDividesTheDeltaByTheStep(): void {
+		$thold = $this->threshold(['data_source_type_id' => self::DERIVE, 'oldvalue' => 100]);
+
+		$this->assertEqualsWithDelta(2, $this->currentValue($thold, 700), 1.0e-9);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testNonNumericReadingYieldsTheNoValueSentinel(): void {
+		$this->assertSame('', $this->currentValue($this->threshold(), 'U'));
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testMissingDataSourceYieldsTheNoValueSentinel(): void {
+		$thold          = $this->threshold();
+		$reindexed      = [];
+		$time_reindexed = [4 => 1700000300];
+		$item           = [];
+		$currenttime    = 0;
+
+		$this->assertSame('', thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime));
+	}
+}

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"]

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:

thold_functions.php

@@ -772,6 +772,37 @@ function thold_expression_specialtype_rpn($operator, &$stack, $local_data_id, $c
 	}
 }
 
+/**
+ * Counts a wrapped counter has advanced by, given the previous and current
+ * readings.
+ *
+ * The modulus is 2^32 or 2^64, not one less than it, so the previous code lost
+ * exactly one count per wrap. 2^64 is above PHP_INT_MAX and would be parsed as
+ * a float, losing about eleven bits at that magnitude, so the 64-bit case goes
+ * through GMP. Cacti already requires ext-gmp.
+ *
+ * @param float|int|string $oldvalue Previous reading.
+ * @param float|int|string $newvalue Current reading.
+ *
+ * @return float|int
+ */
+function thold_counter_wrap_delta($oldvalue, $newvalue) {
+	if ($oldvalue > 4294967295) {
+		$old_integer = trim((string) $oldvalue);
+		$new_integer = trim((string) $newvalue);
+
+		if (!preg_match('/^\d+$/D', $old_integer) || !preg_match('/^\d+$/D', $new_integer)) {
+			return (18446744073709551616.0 - (float) $oldvalue) + (float) $newvalue;
+		}
+
+		$delta = gmp_add(gmp_sub(gmp_pow(2, 64), gmp_init($old_integer, 10)), gmp_init($new_integer, 10));
+
+		return (float) gmp_strval($delta);
+	}
+
+	return (4294967296 - $oldvalue) + $newvalue;
+}
+
 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']];
@@ -798,17 +829,14 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe
 		if (isset($item[$thold_data['name']]) && is_numeric($item[$thold_data['name']])) {
 			switch ($thold_data['data_source_type_id']) {
 				case 2:	// COUNTER
-					if ($thold_data['oldvalue'] != 0 && is_numeric($thold_data['oldvalue'])) {
+					// A previous reading of zero is a real reading, not a missing one.
+					if (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'];
 						} else {
 							// Possible overflow, see if its 32bit or 64bit
-							if ($thold_data['oldvalue'] > 4294967295) {
+							$currentval = thold_counter_wrap_delta($thold_data['oldvalue'], $item[$thold_data['name']]);
-								$currentval = (18446744073709551615 - $thold_data['oldvalue']) + $item[$thold_data['name']];
-							} else {
-								$currentval = (4294967295 - $thold_data['oldvalue']) + $item[$thold_data['name']];
-							}
 						}
 
 						if (strpos($thold_data['rrd_maximum'], '|query_') !== false) {
@@ -1201,9 +1229,11 @@ function thold_calculate_percent($thold, $currentval, $rrd_reindexed) {
 	}
 
 	if (isset($rrd_reindexed[$thold['local_data_id']][$ds])) {
-		$t = (int) $rrd_reindexed[$thold['local_data_id']][$thold['percent_ds']];
+		// Not cast to int: a denominator below one truncated to zero, which
+		// forced the percentage to zero and kept a low threshold alerting.
+		$t = $rrd_reindexed[$thold['local_data_id']][$thold['percent_ds']];
 
-		if ($t > 0) {
+		if (is_numeric($t) && $t != 0) {
 			$currentval = ($currentval / $t) * 100;
 		} else {
 			$currentval = 0;

thold_process.php

@@ -207,10 +207,12 @@ while (true) {
 					$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']])) {
-					$lasttime = $item[$thold_data['name']];
+					$rawvalue = $item[$thold_data['name']];
 				} else {
-					$lasttime = $currenttime - $thold_data['rrd_step'];
+					$rawvalue = $thold_data['oldvalue'];
 				}
 
 				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);
@@ -219,7 +221,7 @@ while (true) {
 					SET tcheck = 1, lastread = ?,
 					lasttime = FROM_UNIXTIME(?), oldvalue = ?
 					WHERE id = ?',
-					[$currentval, $currenttime, $lasttime, $thold_data['thold_id']]
+					[$currentval, $currenttime, $rawvalue, $thold_data['thold_id']]
 				);
 			}