You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Show filter enabled status and repair legacy state schema - #296
The filter Enable/Disable actions reference a column missing from the installation schema, and the list does not show filter state. Add that state for fresh installs and repair legacy tables through the background CLI upgrader, including same-version installations. Existing state is preserved; legacy filters default to enabled.
Show Enabled in green and Disabled in grey using existing theme classes. Disabled filters are excluded from report and new-schedule choices. Editing an existing schedule preserves and labels its disabled filter. Missing or disabled filters skip preparation successfully.
Schema repair
Guarded pages return a retry response while migration is pending. They neither execute DDL nor load the repair function. Readiness checks use uncached metadata through the configured FlowView connection. The background upgrader releases only its own PID-scoped registration on success, failure and same-version exits; refused registrations cannot remove another owner.
Report lifecycle
Both supported Cacti entrypoints use one FlowView dispatcher. It claims only pending occurrences and requires exactly one affected row. Lost claims cannot launch children or reset/delete another owner's work. Non-FlowView sources retain their existing legacy path.
The child records a durable complete marker after successful preparation/delivery and saved schedule timestamps, or retry for an explicit pre-delivery deferral. Successful empty results complete normally. Queue/schedule/filter lookups, partition metadata and query failures preserve retry. Invalid parallel components and failed intermediate CREATE/INSERT operations cannot produce successful partial reports. Failed/timed-out parallel requests are invalidated so retry creates fresh shards.
Every started child requires a recorded outcome, including exit 0 or 75. A timeout, crash or early exit alone proves neither delivery nor safe retry. The dispatcher reconciles the child's recorded outcome; unresolved work remains running and logs its queue ID for verification instead of automatically resending. A distinct startup sentinel allows retry when no process could start. Completion cleanup and executable pending/retry work have separate bounded budgets. Cleanup runs first without an execution-capacity requirement; persistent deletion failures cannot consume the executable budget. Completed markers are deleted without execution. Explicit retry markers return to pending with conditional updates, so an older parent cannot overwrite a newer claim. Persistent completion-deletion failure logs repair instructions without resending. Post-delivery timestamp-write failure also retains the occurrence for repair.
Each bounded snapshot attempts an occurrence once; new work precedes previously attempted failures. Process checks retain live/uncertain registrations and reclaim only confirmed dead PIDs with an affected-row check. Child, master and upgrader shutdown cleanup is PID-scoped. README explains the queue states and safe manual reconciliation.
Validation
Latest revision: 537dca1. Local PHP 8.3 / Cacti 1.2.29: 259 tests / 1,967 assertions. Configured changed-line coverage: 100% (177/177 included production lines); six changed entrypoint/setup files are outside Clover and have separate subprocess regressions. PHP syntax, manifest, translation and whitespace checks pass.
Tests include actual page/upgrader/schedule entrypoints, native/legacy arguments, uncached serial/parallel queries, metadata and SQL failures, process ownership, completion cleanup, and persisted SQLite fairness/claim races. Real Cacti process-helper tests reproduce stderr with exit zero and stderr with timeout/late completion. An actual dedicated-database connection-failure child exits zero without a completion marker and is retained by both entrypoints. A persisted batch of 100 undeletable completed rows does not block pending occurrence 101. The timeout fixture reproduced pending resets and a second claim against the prior revision, then verifies one delivery and late cleanup with this change. Timestamp persistence failure after completion is held for repair. SQLite/database fixtures and simulated delivery do not validate SMTP or MariaDB process concurrency.
Earlier isolated MariaDB 11.8.9 tests with real Cacti functions passed legacy repair, uncached metadata refresh, defaults, repeat upgrades, disabled-state preservation, schedule associations and enabled-only choices. Those tests predate the latest lifecycle fixes. A latest-revision live MariaDB rerun, full upstream Actions approval/run and complete focused-branch browser layout review remain outstanding. Earlier PHP 8.2/8.4 runs also predate this revision.
The reason will be displayed to describe this comment to others. Learn more.
Addressed in ce14e35. Page requests no longer call the schema repair. They check readiness, request the existing background upgrade path for same-version repairs, and return a translated HTTP 503 retry message while the column is unavailable. ALTER runs in flowview_upgrade.php after process registration; failed repair exits unsuccessfully for retry. Scheduled reports also defer without running DDL. The shared page handler avoids repeating this logic across entry points.
The reason will be displayed to describe this comment to others. Learn more.
Rechecked on bed4004. The ALTER remains exclusively behind flowview_upgrade.php; web entrypoints only check readiness and launch the background worker. Added subprocess regressions in ca7e07f executing the actual flowview.php, flowview_filters.php and flowview_schedules.php entrypoints: each returns HTTP 503 plus the retry message without DDL while the column is missing, and continues normally when ready. Actual same-version upgrade entrypoint tests verify registration before repair, nonzero exit on repair failure, and no DDL when registration is refused or the column already exists. These tests use isolated core/database API fixtures; fresh live MariaDB validation remains separate.
The reason will be displayed to describe this comment to others. Learn more.
Follow-up in 8f9e856 makes the CLI/background boundary explicit: the mutation function has been removed from shared includes/database.php and moved to includes/filter_upgrade.php, which production code loads only from flowview_upgrade.php after Cacti's CLI bootstrap. Ordinary flowview.php, flowview_filters.php and flowview_schedules.php requests do not load that repair function at all. They retain the readiness check/background-launch/HTTP 503 path.
The actual-entrypoint regressions now assert repair_loaded=false on all three pages, both while pending and while ready; the same-version CLI upgrade tests assert it is loaded there, with registration before any ALTER, nonzero failure for failed repairs, and no ALTER if registration is refused or the column already exists. Existing uncached metadata and concurrent-upgrade tests remain passing. No scheduled/page path calls the repair helper.
Validation on PHP 8.3/Cacti 1.2.29: 157 tests / 1,537 assertions; configured changed-line gate 100% (93/93 included lines); manifest/gettext/whitespace checks pass. These are isolated API/entrypoint tests, not a new live MariaDB metadata-lock test. Latest live integration and full upstream CI remain pending. The reviewer thread is left open for confirmation.
The reason will be displayed to describe this comment to others. Learn more.
Current-head verification: rechecked 537dca1 against this exact finding. includes/database.php only checks live column metadata. The ALTER is isolated in includes/filter_upgrade.php, required only by the background CLI flowview_upgrade.php in production. All three web entrypoints call the readiness guard, which requests background repair (including same-version repair), returns 503 and exits instead of executing the ALTER or querying the missing enabled column.
Fresh full local PHP 8.3/Cacti 1.2.29 run: 259 tests / 1,967 assertions. Actual web-entrypoint regressions assert no DDL and that the repair function is not loaded; actual upgrader regressions cover successful/failed same-version repair and PID-scoped registration cleanup. Configured changed-line gate 100% (177/177 included production lines); entrypoint files are outside Clover and covered separately by subprocess tests. The code defect described here is fixed; human review, latest live MariaDB concurrency validation and full upstream CI remain separate outstanding requirements. Please reassess this outdated finding against the current head.
File-scope get_timespan stub leaks across Pest tests
tests/Unit/FilterStatusTest.php:71
This installs an always-throwing get_timespan() during Pest collection, leaving it active for the entire test process rather than only the enabled-schedule test. Any other test calling the helper inherits the exception; if the real helper is already loaded, this test instead loses its expected sentinel. Move these core-helper doubles into the shared bootstrap with resettable, configurable behavior, and enable the exception only in the enabled-schedule test. This also follows the repository's explicit restriction on file-scope core-function stubs.
Also resolved the review's file-scope test-helper finding in df7549a: get_timespan() and read_user_setting() are now shared bootstrap doubles with resettable defaults. Only the enabled-schedule test queues the exception sentinel, and a regression verifies reset restores ordinary get_timespan behavior. This avoids leaking an always-throwing helper into unrelated tests. The updated suite passes all 116 tests on PHP 8.2, 8.3 and 8.4. Follow-up 3d38365 also preserves exec_with_timeout’s real return contract in the shared test double, fixing the resulting static-analysis finding.
Handle and log queue-repair failures after child errors
includes/functions-pre13.php:301
Check this reset's result and log the queue-repair error, as flowview_reports_run() does. If the write fails after a failed FlowView child, the legacy runner retains the occurrence as running, excluding it from pending scans, but does not tell operators that queue repair is required.
Resolved the legacy-reset logging finding in 3ced852. The pre-1.3 compatibility runner now checks the retry-state UPDATE result and logs the same explicit "queue repair is required" error as the FlowView-owned dispatcher if it fails. The original occurrence remains retained. The shared parent-completion regression now includes a failed legacy reset and asserts the repair log and absence of queue deletion.
Additional recheck fixed in ceff2b4. There were no new inline findings after bd9702a; inspection of the two-request path found that intermediate CREATE/INSERT failures were ignored. That could still reduce partial data and complete the scheduled occurrence successfully.
The helper now stops materialization on the first failed write, skips final reduction, drops the temporary table, and returns false through common failed-request cleanup. The retained scheduled occurrence can recreate its requests and retry. Added four regressions: failed table creation, failed first insert, failed second insert, and successful nonempty reduction. All three failure cases reproduced false success before the fix; tests assert no final reduction, no writes after the failing component except cleanup, temporary-table cleanup and failed-request invalidation.
Latest local PHP 8.3/Cacti 1.2.29: 209 tests / 1,745 assertions; configured changed-line gate 100% (159/159 included production lines); manifest/gettext/whitespace pass. Six changed entrypoint/setup files remain outside Clover instrumentation. Latest live MariaDB validation/full upstream CI remain pending. This is an additional code-check finding, not a new Copilot finding; reviewer-thread state is unchanged.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
A duplicate-delivery retry defect remains unresolved, and migration and queue changes still need latest-revision live MariaDB validation and upstream CI confirmation.
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
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.
The filter Enable/Disable actions reference a column missing from the installation schema, and the list does not show filter state. Add that state for fresh installs and repair legacy tables through the background CLI upgrader, including same-version installations. Existing state is preserved; legacy filters default to enabled.
Show Enabled in green and Disabled in grey using existing theme classes. Disabled filters are excluded from report and new-schedule choices. Editing an existing schedule preserves and labels its disabled filter. Missing or disabled filters skip preparation successfully.
Schema repair
Guarded pages return a retry response while migration is pending. They neither execute DDL nor load the repair function. Readiness checks use uncached metadata through the configured FlowView connection. The background upgrader releases only its own PID-scoped registration on success, failure and same-version exits; refused registrations cannot remove another owner.
Report lifecycle
Both supported Cacti entrypoints use one FlowView dispatcher. It claims only pending occurrences and requires exactly one affected row. Lost claims cannot launch children or reset/delete another owner's work. Non-FlowView sources retain their existing legacy path.
The child records a durable
completemarker after successful preparation/delivery and saved schedule timestamps, orretryfor an explicit pre-delivery deferral. Successful empty results complete normally. Queue/schedule/filter lookups, partition metadata and query failures preserve retry. Invalid parallel components and failed intermediate CREATE/INSERT operations cannot produce successful partial reports. Failed/timed-out parallel requests are invalidated so retry creates fresh shards.Every started child requires a recorded outcome, including exit 0 or 75. A timeout, crash or early exit alone proves neither delivery nor safe retry. The dispatcher reconciles the child's recorded outcome; unresolved work remains running and logs its queue ID for verification instead of automatically resending. A distinct startup sentinel allows retry when no process could start. Completion cleanup and executable pending/retry work have separate bounded budgets. Cleanup runs first without an execution-capacity requirement; persistent deletion failures cannot consume the executable budget. Completed markers are deleted without execution. Explicit retry markers return to pending with conditional updates, so an older parent cannot overwrite a newer claim. Persistent completion-deletion failure logs repair instructions without resending. Post-delivery timestamp-write failure also retains the occurrence for repair.
Each bounded snapshot attempts an occurrence once; new work precedes previously attempted failures. Process checks retain live/uncertain registrations and reclaim only confirmed dead PIDs with an affected-row check. Child, master and upgrader shutdown cleanup is PID-scoped. README explains the queue states and safe manual reconciliation.
Validation
Latest revision: 537dca1. Local PHP 8.3 / Cacti 1.2.29: 259 tests / 1,967 assertions. Configured changed-line coverage: 100% (177/177 included production lines); six changed entrypoint/setup files are outside Clover and have separate subprocess regressions. PHP syntax, manifest, translation and whitespace checks pass.
Tests include actual page/upgrader/schedule entrypoints, native/legacy arguments, uncached serial/parallel queries, metadata and SQL failures, process ownership, completion cleanup, and persisted SQLite fairness/claim races. Real Cacti process-helper tests reproduce stderr with exit zero and stderr with timeout/late completion. An actual dedicated-database connection-failure child exits zero without a completion marker and is retained by both entrypoints. A persisted batch of 100 undeletable completed rows does not block pending occurrence 101. The timeout fixture reproduced pending resets and a second claim against the prior revision, then verifies one delivery and late cleanup with this change. Timestamp persistence failure after completion is held for repair. SQLite/database fixtures and simulated delivery do not validate SMTP or MariaDB process concurrency.
Earlier isolated MariaDB 11.8.9 tests with real Cacti functions passed legacy repair, uncached metadata refresh, defaults, repeat upgrades, disabled-state preservation, schedule associations and enabled-only choices. Those tests predate the latest lifecycle fixes. A latest-revision live MariaDB rerun, full upstream Actions approval/run and complete focused-branch browser layout review remain outstanding. Earlier PHP 8.2/8.4 runs also predate this revision.