refactor(poller): resilient dual-stack map generation - #111
Open
somethingwithproof wants to merge 5 commits into
Open
refactor(poller): resilient dual-stack map generation#111somethingwithproof wants to merge 5 commits into
somethingwithproof wants to merge 5 commits into
Conversation
TheWitness
previously approved these changes
Aug 17, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the gpsmap poller workflow to load the mapped Device set once per poller cycle and reuse it across subnet renders, significantly reducing repeated DB queries and DNS lookups. It also hardens upgrade/version persistence behavior, makes artefact publication behavior explicit on failure vs empty estates, improves atomic artefact writes, and unifies icon validation between the UI dropdown and the renderers.
Changes:
- Split region processing into
gpsmap_load_devices()(single load) andgpsmap_render_region()(per-subnet render), with prefix enumeration derived from already-resolved addresses. - Add/extend tests and coverage targets for polling + upgrade behavior, and add new upgrade-path regression tests.
- Move/centralize icon enumeration logic into
gpsmap_security.phpand align it with renderer constraints.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_upgrade.php | New regression tests for upgrade gating, version persistence, and backoff behavior. |
| tests/test_polling.php | Adds tests for single-pass loading, atomic writes, publication decisions, and direct poller entry point coverage. |
| tests/test_icons.php | Adds tests ensuring getIcons() matches renderer constraints and respects base_path. |
| tests/run.php | Adds test_upgrade.php to the test runner. |
| tests/harness.php | Ensures test tmp root symlinks include additional plugin entrypoints needed by new tests. |
| tests/coverage.php | Expands coverage gate to include polling + upgrade logic; removes deleted entry point mention. |
| setup.php | Passes persisted old-version into gpsmap_upgrade_database() instead of relying on an unset global. |
| print.php | Clarifies authentication control flow in comment. |
| includes/towerSelect.php | Removes unused/unreferenced legacy entry point. |
| includes/setup/database.php | Makes upgrade path deterministic (version persisted, retry backoff, schema verification) and adjusts table create details. |
| includes/polling/processregion.php | Introduces load/render split, prefix derivation, and device state reset between renders. |
| includes/polling/functions.php | Makes artefact writes staged+atomic while preserving destination file mode. |
| includes/polling.php | Refactors poller entry to single-load per cycle + explicit publish/withhold decisions and stats logging. |
| gpstemplates.php | Uses shared getIcons() (now in gpsmap_security.php) and escapes edit URL. |
| gpsmap_security.php | Adds getIcons() implementation consistent with renderer constraints and base_path. |
| .gitignore | Ignores local debug file under tests. |
Suppressed comments (1)
tests/test_polling.php:341
- This base_path override is immediately restored without any assertion or code that depends on it, so it has no effect (and can confuse future readers). It should be removed or turned into a real assertion.
$savedRoot = $GLOBALS['config']['base_path'];
$GLOBALS['config']['base_path'] = $root . '/no-such-root';
$GLOBALS['config']['base_path'] = $savedRoot;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ubnet region() re-queried every Device and re-resolved every hostname on each call, and the poller called it once per discovered subnet prefix. On a 2000-Device install across 150 subnets that was 174 table scans and roughly 348,000 DNS lookups per cycle. Split into gpsmap_load_devices() and gpsmap_render_region() so the load runs once and every subnet renders from that set. A failed Device query withholds publication, because overwriting the artefacts with an empty set would blank the map; an estate with genuinely no mapped Devices still publishes, so a new install gets an all.xml rather than a 404. Artefact writes are staged and renamed, preserving the destination mode, so a reader never sees a truncated or unreadable document. Closes Cacti#6 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
$old was read from an unset global, so every migration re-ran on each version change. Two ALTER statements backticked a literal default, which MySQL reads as an identifier, so they had been failing silently. And the version was recorded before the migrations ran, so a failed ALTER left the schema behind while the plugin reported itself current. The schema is now verified with db_column_exists() rather than trusting helper return values, both version records are written together only on success, and a failure backs off instead of re-running an ALTER on host from every page view. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
getIcons() accepted my-icon.png while the JavaScript emitter rejected it, so the icon appeared in the Map Template dropdown, saved cleanly, and then never rendered. Both sides now use gpsmap_icon_identifier(), the path is built from base_path so poller and CLI callers resolve it, and a missing icon directory no longer prints a PHP warning into the Map Templates form. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Nothing in the plugin calls it, so it was an authenticated route to host_template that no page uses. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
force-pushed
the
refactor/poller-architecture
branch
from
August 17, 2026 02:20
bc05aa7 to
3fdb4a8
Compare
This was referenced Aug 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The poller now loads the Device set once and publishes resilient dual-stack snapshots:
/16,/32,/48IPv6 drill-downalertand uses their Down iconIssue coverage and overlap
This PR is the single implementation PR for the related poller concerns. No separate PR duplicates these changes:
PR #102 changes workflows only and does not overlap these runtime changes.
Linux Docker validation
git diff --check: cleanThe repository has no
composer.json; none was created or edited.