fix: backport spikekill, aggregate, and realtime graph fixes to 1.2.x by somethingwithproof · Pull Request #6905 · Cacti/cacti

somethingwithproof · 2026-03-28T20:55:35Z

Summary

Swap inverted user/default assignments in spikekill initializeSpikekill()
Separate SIMILAR from ALL in text_format and pparts replacements
Emit error image for realtime graph when RRD file missing
Read gprint_format when creating aggregate graphs
Add unit tests

Test plan

Verify spikekill processes with correct user/default settings
Verify aggregate graphs render with correct 95th percentile
Verify realtime graph shows error image when RRD missing
Run vendor/bin/pest tests/Unit/SpikekillInitTest.php tests/Unit/AggregatePtileTypeTest.php tests/Unit/AggregateGprintFormatTest.php tests/Unit/RrdFileExistsCheckTest.php

Copilot

Pull request overview

Backports several fixes to the Cacti 1.2.x branch around spikekill initialization, aggregate graph formatting/percentile handling, and realtime graph rendering when RRD files are missing, along with adding new unit tests.

Changes:

Fix spikekill initialization to prefer user settings over system defaults when constructor values are empty.
Add an early missing-RRD check in rrdtool_function_graph() and attempt to return an error image instead of repeatedly retrying RRDtool.
Add new Pest unit tests intended to cover spikekill initialization, aggregate SIMILAR/ALL percentile replacements, aggregate gprint_format mapping, and the missing-RRD behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
`lib/spikekill.php`	Swaps inverted user/default assignments in `initialize_spikekill()`.
`lib/rrd.php`	Adds missing-RRD early handling in graph generation; also introduces several comment typos.
`lib/functions.php`	Removes path validation helpers and introduces several comment typos.
`lib/api_aggregate.php`	Minor comment text change (but aggregate fix described in PR is not present in the code shown).
`tests/Unit/SpikekillInitTest.php`	Adds tests for spikekill user/default assignment logic, but currently targets the wrong field set.
`tests/Unit/RrdFileExistsCheckTest.php`	Adds source-scanning tests for the missing-RRD existence check.
`tests/Unit/AggregatePtileTypeTest.php`	Adds tests for SIMILAR vs ALL replacement behavior; currently references a missing stub file and doesn’t match production logic.
`tests/Unit/AggregateGprintFormatTest.php`	Adds tests for gprint_format propagation in aggregate creation; production mapping change is not present in `graphs.php`.

Copilot · 2026-03-28T21:00:53Z

lib/functions.php


 	/**
-	 * Handle the case where the Email is a string, but may
+	 * Handle the case where the Email is a tring, but may


Typo in comment: "tring" should be "string".

Suggested change

* Handle the case where the Email is a tring, but may

* Handle the case where the Email is a string, but may

Copilot · 2026-03-28T21:00:53Z

lib/functions.php


 /**
- * cacti_cookie_set - Allows for settings an arbitrary cookie name and value
+ * cacti_cookie_set - Allows for settings an arbitry cookie name and value


Typo in docblock: "arbitry" should be "arbitrary".

Suggested change

* cacti_cookie_set - Allows for settings an arbitry cookie name and value

* cacti_cookie_set - Allows for settings an arbitrary cookie name and value

Copilot · 2026-03-28T21:00:54Z

lib/functions.php


 /**
- * cacti_time_zone_set - Given an offset in minutes, attempt
+ * cacti_time_zone_set - Givin an offset in minutes, attempt


Typo in docblock: "Givin" should be "Given".

Suggested change

* cacti_time_zone_set - Givin an offset in minutes, attempt

* cacti_time_zone_set - Given an offset in minutes, attempt

Copilot · 2026-03-28T21:00:54Z

lib/functions.php

+ * cacti_time_zone_set - Givin an offset in minutes, attempt
 * to set a PHP date.timezone.  There are some oddballs that
- * we have to accommodate.
+ * we have to accomodate.


Typo in docblock: "accomodate" should be "accommodate".

Suggested change

* we have to accomodate.

* we have to accommodate.

Copilot · 2026-03-28T21:00:54Z

lib/rrd.php

+						return false;
+					}
+
+					return rrdtool_create_error_image(__('The Cacti Poller has not run yet.'));


This change returns rrdtool_create_error_image() (a PNG payload) directly from rrdtool_function_graph() when the RRD file is missing. Callers like graph_image.php send the response Content-Type based on the requested image format (e.g., image/svg+xml), so returning PNG bytes here can produce an incorrect content-type/body mismatch. Consider returning false (and emitting an error string for get_error mode) so existing caller-side error handling can generate an error image with the correct headers, or generate an error response that matches $graph_data_array['image_format'].

Suggested change

return rrdtool_create_error_image(__('The Cacti Poller has not run yet.'));

if (isset($graph_data_array['get_error'])) {

return __('The Cacti Poller has not run yet.');

}

return false;

Copilot · 2026-03-28T21:00:55Z

lib/rrd.php

 				}

-				/* check the minimal heartbeat */
+				/* check the mimimal heartbeat */


Typo in comment: "mimimal" should be "minimal".

Suggested change

/* check the mimimal heartbeat */

/* check the minimal heartbeat */

Copilot · 2026-03-28T21:00:55Z

tests/Unit/SpikekillInitTest.php

+// The six spikekill fields and their system defaults (from spikekill class properties)
+define('SPIKEKILL_FIELDS', [
+	'avgnan'   => 'last',
+	'method'   => '1',
+	'numspike' => '10',
+	'stddev'   => '10',
+	'dsfilter' => '',
+	'absmax'   => '1000000000',
+]);


The test data here doesn't match the actual spikekill fields/defaults used by spikekill::initialize_spikekill() in lib/spikekill.php (it uses method/avgnan/stddev/numspike/percent/outliers, not dsfilter/absmax). As written, these tests won't validate the behavior changed in this PR (especially percent/outliers) and may give false confidence. Please align the tested field set and defaults with the spikekill class properties and update the narrative references (e.g., initialize_spikekill() naming) accordingly.

Copilot · 2026-03-28T21:00:56Z

tests/Unit/AggregatePtileTypeTest.php

+ * Tests for the AGGREGATE_TOTAL_TYPE_SIMILAR percentile replacement fix.
+ *
+ * Prior to the fix, the SIMILAR case was grouped with ALL in three
+ * str_replace blocks, causing SIMILAR to use aggregate_sum / aggregate_sum_peak
+ * (summed across all data sources) instead of aggregate_current / aggregate_current_peak
+ * (per-data-source percentiles). This produced identical inbound and outbound
+ * 95th percentile HRULEs.
+ *
+ * See: https://github.com/Cacti/cacti/issues/6470
+ *
+ * The fix separates SIMILAR into its own elseif branch in all three locations:
+ *   1. text_format :current: / :max: replacement (aggregate_graphs_insert_graph_items)
+ *   2. COMMENT item pparts[3] replacement (aggregate_graphs_insert_graph_items)
+ *   3. HRULE item pparts[3] replacement (aggregate_graphs_insert_graph_items)
+ */


The PR description and this test file describe separating AGGREGATE_TOTAL_TYPE_SIMILAR from AGGREGATE_TOTAL_TYPE_ALL in the replacement logic, but lib/api_aggregate.php still groups SIMILAR with ALL (e.g., conditions like "== ALL || == SIMILAR" in aggregate_handle_ptile_type()). Either update the production code to match the intended behavior or adjust these tests so they reflect the actual implementation.

Copilot · 2026-03-28T21:00:56Z

lib/api_aggregate.php

 		$graph_data['title_cache'] = $existing_data['title_cache'];
 	} else {
 		/* this is an existing graph and not templated from aggregate,
-		 * reuse its old data */
+		 * re-use its old data */
 		$graph_data = $existing_data;
 	}


The PR description mentions the SIMILAR vs ALL percentile replacement fix, but this file’s functional logic still treats SIMILAR like ALL in several places (e.g., conditions of the form "== AGGREGATE_TOTAL_TYPE_ALL || == AGGREGATE_TOTAL_TYPE_SIMILAR"). If SIMILAR is supposed to use per-data-source (aggregate_current*) functions, those conditions need to be split into distinct branches so SIMILAR does not map to aggregate_sum/aggregate_sum_peak.

Copilot · 2026-03-28T21:00:56Z

tests/Unit/AggregatePtileTypeTest.php

+ *   3. HRULE item pparts[3] replacement (aggregate_graphs_insert_graph_items)
+ */
+
+require_once dirname(__DIR__) . '/Helpers/CactiStubs.php';


This test requires "tests/Helpers/CactiStubs.php", but there is no tests/Helpers directory in this branch (only tests/Unit and tests/tools). As a result, running this test will fatal with a missing file. Either add the stub file at that path, or remove the require and adjust the test to run with the existing bootstrap/autoload setup.

Suggested change

require_once dirname(__DIR__) . '/Helpers/CactiStubs.php';

somethingwithproof · 2026-03-28T21:05:29Z

Bug fixes backported from develop:

Spikekill not working as expected #5266 - Spikekill inverted user/default assignments in initializeSpikekill()
Why do the aggregated ports in Figure 1 have the same input and output directions (95 values), while the single-port ports in Figure 2 work normally? What could be the reason for this? #6470 - Aggregate SIMILAR incorrectly treated as ALL in text_format/pparts
1.3 - RRD issue after creating graph #6530 - Realtime graph crashes when RRD file is missing (now shows error image)
Aggregate gprint_format not read when creating graphs

Verification: PHP lint clean, PHPStan level 5 clean, Pest tests pass.

- Swap inverted user/default assignments in spikekill initializeSpikekill() - Separate SIMILAR from ALL in text_format and pparts replacements - Emit error image for realtime graph when RRD file missing - Read gprint_format when creating aggregate graphs - Add unit tests for spikekill, aggregate, and RRD checks Fixes Cacti#5266, Cacti#6470, Cacti#6530 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

TheWitness · 2026-03-28T22:27:23Z

lib/functions.php

 * @param $level - (int) only log if above the specified log level
 */
-function cacti_log($string, $output = false, $environ = 'CMDPHP', $level = '') {
+function cacti_log($sstring, $output = false, $environ = 'CMDPHP', $level = '') {


Jacked up here. "sstring"?

TheWitness · 2026-03-28T22:27:40Z

lib/functions.php

- * @param  (string) The actual error message to display
+ * @param  (sstring) The title for the dialog title bar
+ * @param  (sstring) Header section for the message
+ * @param  (sstring) The actual error message to display


Oh my goodness everywhere.

somethingwithproof · 2026-03-28T22:39:31Z

Closing - lib/functions.php checkout reverted recent changes. Will redo with only the targeted files.

Copilot AI review requested due to automatic review settings March 28, 2026 20:55

Copilot started reviewing on behalf of somethingwithproof March 28, 2026 20:56 View session

Copilot AI reviewed Mar 28, 2026

View reviewed changes

somethingwithproof force-pushed the backport/1.2.x-bug-fixes-batch branch from 7f9b66e to faca8ad Compare March 28, 2026 22:24

TheWitness requested changes Mar 28, 2026

View reviewed changes

somethingwithproof closed this Mar 28, 2026

	* Handle the case where the Email is a tring, but may
	* Handle the case where the Email is a string, but may

	* cacti_cookie_set - Allows for settings an arbitry cookie name and value
	* cacti_cookie_set - Allows for settings an arbitrary cookie name and value

	* cacti_time_zone_set - Givin an offset in minutes, attempt
	* cacti_time_zone_set - Given an offset in minutes, attempt

	/* check the mimimal heartbeat */
	/* check the minimal heartbeat */

Uh oh!

Conversation

somethingwithproof commented Mar 28, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof commented Mar 28, 2026

Uh oh!

TheWitness Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

TheWitness Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof commented Mar 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants