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: 59d8b6b. Local PHP 8.3 / Cacti 1.2.29: 341 tests / 2,372 assertions. Configured changed-line coverage: 100% (229/229 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.
Synced with upstream develop at c3123ef, preserving its CI dependency/security updates and merged DNS fix. Resolved the changelog conflict without dropping either entry.
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.
Scheduled reports use a FlowView-owned delivery helper on Cacti 1.2 and 1.3, rather than relying on core’s void notification result. Its actual delivery queue lookup returns false only before notification/logging starts. Mailer error strings (including partial multi-target delivery), invalid notification settings and failed report-log persistence return null: the child exits 76 without timestamps or completion/retry markers, and the parent holds the occurrence for repair. Only successful delivery and log persistence permit completion. The legacy compatibility name delegates to the same helper. Notification lists use plugin_notification_lists, matching the schedule UI.
Regressions cover both launch forms and dispatchers, direct/list mailer error returns, partial multi-notification delivery, report-log failure, malformed notifications, failed/missing queue rows and successful completion. Native-core cases load the official Cacti develop notification helper (lib/reports.php revision 41e6d9e03336139791e2660b12df444d45e6eb58) in the actual child, assert its presence, and verify the shared FlowView path exposes outcomes without invoking the opaque helper. Run these cases on Cacti 1.3 or set FLOWVIEW_NATIVE_REPORTS_SOURCE to an official native lib/reports.php; they skip when the configured core lacks the native helper. The local run supplied that source and ran all 341 tests without skips. Mail/DB boundaries are simulated; this is not a live SMTP or full Cacti 1.3 integration result.
Recipient compatibility: direct email and notification lists accept any nonempty To/CC/BCC combination. A 32-case recipient/error matrix and four actual-child CC/BCC-only regressions protect this contract; the previous helper failed 16 of those targeted cases.
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.
Latest recheck found PR #296 was conflicting with develop, preventing current-revision integration workflow scheduling. Fixed in merge commit ffbf5a4: merged upstream develop c3123ef, kept its CI dependency/security updates and DNS fix, and resolved the sole changelog conflict while preserving both feature/fix entries and upstream history. PR #297 was not changed.
Merged-candidate validation: PHP 8.3/Cacti 1.2.29, 260 tests / 1,988 assertions; configured changed-line gate against upstream/develop 100% (177/177 included production lines); manifest/gettext/whitespace pass. Six entrypoint/setup files remain outside Clover and have separate subprocess regressions.
The two previously linked code defects remain fixed, but their review threads are left unresolved. Latest live MariaDB concurrency validation and human review are still pending. Local MariaDB/container tooling is unavailable and the previously used Cacti test-server session is not open, so no new live-database result is claimed. Earlier Actions runs are action_required; the integration workflow must be approved/run for this latest revision. This update addresses mergeability, not a claim of merge readiness.
Mailer errors are discarded, falsely marking delivery complete
includes/functions-pre13.php:185
Cacti 1.2.29's mailer() returns an error string for SMTP/address failures rather than necessarily throwing. Both mailer calls above discard that result, so this unconditional success lets the child record complete and the dispatcher delete an occurrence despite failed notification. Check both mailer results and propagate a distinct unresolved-delivery outcome so the occurrence remains held for repair. Do not simply return false, since the caller treats that as safe to retry and delivery may be partial. Add a regression where the mailer returns an error without throwing.
On Cacti 1.3, the native reports_log_and_notify() returns void even when its own queue SELECT fails or finds no row, skipping notification and report logging. null !== false therefore reports success here: the child saves schedule timestamps and records complete, allowing the undelivered occurrence to be deleted. The new subprocess tests use the pre-1.3 shim even with native launch arguments. Use a native-compatible notification path that exposes its actual pre-delivery lookup outcome, and test failed and missing lookups with the native helper. A separate preflight SELECT would not protect the helper's later lookup.
Fixed both previously missed notification issues from review #296 (review) in a3bc078.
Returned mailer errors: direct email and notification-list sends now check Cacti’s error string. A returned error produces an unresolved outcome, not a safe retry or completion. The actual child exits 76 without advancing schedule timestamps or writing a complete/retry marker. Both dispatchers retain it without resetting/deleting it. Partial multi-target delivery and failed report-log persistence are also held for manual reconciliation.
Native helper hiding queue lookup failures: scheduled FlowView reports now use one plugin-owned observable delivery path on both versions. The lookup used for delivery itself returns false on SQL failure/missing row before any send/log write; it is not a preflight SELECT followed by core’s opaque helper. The pre-1.3 compatibility name delegates to the same helper. Actual-child tests load the official Cacti develop native notification helper (reports.php revision 41e6d9e03336139791e2660b12df444d45e6eb58), assert it is present, and verify failed/missing lookup retry and successful completion through the independent FlowView path. Native launch arguments alone are no longer treated as evidence of native-core coverage.
Validation: PHP 8.3/Cacti 1.2.29 harness with the official native helper source supplied: 305 tests / 2,214 assertions, no skips. Includes actual child/parent regressions across both launch forms, returned direct/list mail errors, throwing mailer, log persistence failure, partial multi-notification failure and success. Configured changed-line coverage 100% (229/229 included production lines); six entrypoint/setup files are outside Clover and covered separately by subprocess tests. Syntax, manifest, gettext and whitespace checks pass. Native-helper loading is a compatibility fixture, not full Cacti 1.3/SMTP integration. Latest live MariaDB concurrency validation, full upstream Actions and human review remain outstanding. Reviewer thread states and PR #297 unchanged.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Delivery and queue-ownership changes need maintainer review, resolution of the retained defect, and outstanding latest-revision integration validation.
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: 59d8b6b. Local PHP 8.3 / Cacti 1.2.29: 341 tests / 2,372 assertions. Configured changed-line coverage: 100% (229/229 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.
Synced with upstream develop at c3123ef, preserving its CI dependency/security updates and merged DNS fix. Resolved the changelog conflict without dropping either entry.
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.
Scheduled reports use a FlowView-owned delivery helper on Cacti 1.2 and 1.3, rather than relying on core’s void notification result. Its actual delivery queue lookup returns false only before notification/logging starts. Mailer error strings (including partial multi-target delivery), invalid notification settings and failed report-log persistence return null: the child exits 76 without timestamps or completion/retry markers, and the parent holds the occurrence for repair. Only successful delivery and log persistence permit completion. The legacy compatibility name delegates to the same helper. Notification lists use plugin_notification_lists, matching the schedule UI.
Regressions cover both launch forms and dispatchers, direct/list mailer error returns, partial multi-notification delivery, report-log failure, malformed notifications, failed/missing queue rows and successful completion. Native-core cases load the official Cacti develop notification helper (lib/reports.php revision 41e6d9e03336139791e2660b12df444d45e6eb58) in the actual child, assert its presence, and verify the shared FlowView path exposes outcomes without invoking the opaque helper. Run these cases on Cacti 1.3 or set FLOWVIEW_NATIVE_REPORTS_SOURCE to an official native lib/reports.php; they skip when the configured core lacks the native helper. The local run supplied that source and ran all 341 tests without skips. Mail/DB boundaries are simulated; this is not a live SMTP or full Cacti 1.3 integration result.
Recipient compatibility: direct email and notification lists accept any nonempty To/CC/BCC combination. A 32-case recipient/error matrix and four actual-child CC/BCC-only regressions protect this contract; the previous helper failed 16 of those targeted cases.