Skip to content

refactor(poller): resilient dual-stack map generation - #111

Open
somethingwithproof wants to merge 5 commits into
Cacti:developfrom
somethingwithproof:refactor/poller-architecture
Open

refactor(poller): resilient dual-stack map generation#111
somethingwithproof wants to merge 5 commits into
Cacti:developfrom
somethingwithproof:refactor/poller-architecture

Conversation

@somethingwithproof

@somethingwithproof somethingwithproof commented Aug 17, 2026

Copy link
Copy Markdown
Member

Summary

The poller now loads the Device set once and publishes resilient dual-stack snapshots:

  • one Device query per cycle, with every region rendered from that shared set
  • literal IPv4/IPv6 support and filename-safe /16, /32, /48 IPv6 drill-down
  • no synchronous DNS on the poller path; a separate worker refreshes a persistent last-known-good cache
  • optional thold integration marks otherwise-up Devices with active thresholds as alert and uses their Down icon
  • atomic artefact writes plus age-based pruning after three poller intervals, only after a successful Device query
  • deterministic schema upgrades, aligned icon validation, and removal of the unused tower endpoint

Issue 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

  • PHP 8.0 lint: every plugin PHP file passes (vendor excluded)
  • standalone suite: 281 tests, 0 failures
  • Xdebug coverage: 504/504 executable lines, 100%
  • PHP CS Fixer: all 15 changed PHP files clean
  • git diff --check: clean

The repository has no composer.json; none was created or edited.

Copilot AI lite review requested due to automatic review settings August 17, 2026 01:53
@somethingwithproof somethingwithproof added the enhancement New feature or request label Aug 17, 2026
TheWitness
TheWitness previously approved these changes Aug 17, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and gpsmap_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.php and 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.

Comment thread includes/polling/processregion.php Outdated
Comment thread gpsmap_security.php
Comment thread includes/polling.php Outdated
Comment thread tests/test_polling.php Outdated
…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 somethingwithproof changed the title refactor(poller): load the Device set once per cycle instead of per subnet refactor(poller): resilient dual-stack map generation Aug 18, 2026
@somethingwithproof somethingwithproof added the bug Something isn't working label Aug 18, 2026
@somethingwithproof somethingwithproof self-assigned this Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

3 participants