fault_manager: make a time-based confirmation as visible as a reported one - #643
Open
bburda wants to merge 5 commits into
Open
fault_manager: make a time-based confirmation as visible as a reported one#643bburda wants to merge 5 commits into
bburda wants to merge 5 commits into
Conversation
… test it The debounce counter only moves when an event arrives, so confirmation_threshold and healing_threshold only work for a reporter that keeps sending FAILED while a condition is still there. The docs recommended confirmation_threshold: -3 with healing_threshold: 3 to everyone. For a reporter that sends one FAILED per raise and one clear per de-assert the second event never comes. The fault stops at PREFAILED and never confirms, and ListFaults with an empty status filter returns CONFIRMED only, so nobody sees it. Healing breaks the same way: it needs healing_threshold minus confirmation_threshold consecutive PASSED events and only one is sent, so a confirmed fault stays CONFIRMED until someone calls ~/clear_fault. auto_confirm_after_sec is the lever for that kind of reporter and already works. It holds the first FAILED in PREFAILED and confirms it only if it is still there when the window closes, so a condition that recovers never reaches an operator. The docs listed the parameter but never said what it is for. Add an integration test driving the real node over the real services with the event counts such a reporter sends: one FAILED per raise, one PASSED per clear. It also pins the case where a condition clears inside the window, which is what stops a config from passing by only delaying a false alarm. No production code change.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR clarifies fault-manager debounce/healing parameter guidance for edge-triggered (one-event-per-transition) reporters and adds an integration test that exercises the auto_confirm_after_sec + healing_threshold: 0 path end-to-end (SQLite backend), without changing production code.
Changes:
- Document when count-based thresholds work vs when time-based auto-confirm is required (README + Sphinx docs).
- Add a launch-based integration test that drives report/list services with edge-triggered event patterns.
- Register the new integration test in
ros2_medkit_fault_manager’s CMake.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ros2_medkit_fault_manager/test/integration/test_debounce_and_healing.test.py | New launch integration test covering edge-triggered debounce + timer confirm + healing behavior. |
| src/ros2_medkit_fault_manager/README.md | Adds guidance on selecting debounce/healing levers based on reporter behavior. |
| src/ros2_medkit_fault_manager/CMakeLists.txt | Registers the new launch test with an appropriate timeout. |
| docs/config/fault-manager.rst | Adds an “important” note explaining edge-triggered reporter implications and recommended settings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+246
to
+249
| def test_exit_code(self, proc_info, fault_manager_node): | ||
| launch_testing.asserts.assertExitCodes( | ||
| proc_info, allowable_exit_codes=[0, -2, -15], process=fault_manager_node | ||
| ) |
bburda
marked this pull request as draft
August 27, 2026 15:39
…d one A confirmation raised by the auto-confirm timer only reached the database and the audit log. The report path also publishes EVENT_CONFIRMED and enqueues snapshot + rosbag capture, so a fault confirmed by the timer produced no event for subscribers and no black-box recording, while the same fault confirmed by a report produced both. The capture block is now one helper shared by both paths, so they cannot drift apart again. Three smaller fixes on the same surface: auto_confirm_after_sec accepted NaN. Every comparison against NaN is false, so the `< 0.0` guard passed it through and the `> 0.0` timer guard then rejected it, disabling time-based confirmation with nothing logged. A negative value at least warned. Test the positive form and negate it. The startup line reported the requested thresholds rather than the ones in force, so a sanitized value was logged as if it had been accepted. It now reads from the sanitized config. The configuration guide recommended confirmation_threshold: 0 for immediate confirmation, which the node rejects and replaces with -1; it now says -1. bringup_params.yaml paired healing_enabled with healing_threshold: 3 under a comment promising a fault heals when its action recovers. The action bridge emits one PASSED per recovery and healing costs healing_threshold minus the counter the fault confirmed at, so that preset never healed. Threshold 0. Tests: a new integration test asserts the timer publishes EVENT_CONFIRMED, failing without the fix with the event list empty. The healing suite is parametrized over healing_threshold so 0 is shown to be what makes healing reachable, with the run at 3 asserting the latch rather than skipping.
bburda
changed the title
bburda
marked this pull request as ready for review
September 6, 2026 09:28
mfaferek93
reviewed
Sep 6, 2026
| // A timer-driven confirmation is a confirmation: it has to reach the | ||
| // event stream and the black box exactly like one raised by a report, | ||
| // or subscribers see no alarm and no recording is ever made for it. | ||
| publish_fault_event(ros2_medkit_msgs::msg::FaultEvent::EVENT_CONFIRMED, *fault); |
Collaborator
There was a problem hiding this comment.
The timer path publishes for muted symptoms. The report path gates every publish with if (!should_mute) (817, 824, 831); here publish_fault_event(EVENT_CONFIRMED, *fault) runs unconditionally. With a mute_symptoms rule and confirmation_threshold: -2 the symptom sits muted in PREFAILED, the timer confirms it, and SSE / trigger subscriber / entity freeze-frame get a CONFIRMED alarm for a fault handle_list_faults hides.
Suggest skipping the publish when the engine has the code muted (an is_muted(code) on the engine, or get_muted_faults()), and keeping capture_on_confirm unconditional so it matches line 869.
| // disables time-based confirmation with nothing logged. clang-tidy's | ||
| // readability-simplify-boolean-expr suggests the DeMorgan rewrite that puts | ||
| // that back - leave this form alone. | ||
| if (!(std::isfinite(auto_confirm_after_sec_) && auto_confirm_after_sec_ >= 0.0)) { |
Collaborator
There was a problem hiding this comment.
Still lets through finite values that overflow downstream: sqlite_fault_storage.cpp:1087 does static_cast<int64_t>(auto_confirm_after_sec * 1e9), so anything above ~9.2e9 s is UB and cutoff_ns is garbage (memory backend stays in double, so only sqlite). Cheap to reject or clamp above INT64_MAX / 1e9 in this same check, with the same warning.
| For immediate fault confirmation (no debounce), set ``confirmation_threshold: 0``. | ||
| For immediate fault confirmation (no debounce), set ``confirmation_threshold: -1``, | ||
| which is also the default. ``0`` is rejected: the threshold must be strictly | ||
| negative, and the node falls back to ``-1`` with a warning. |
Collaborator
There was a problem hiding this comment.
The node still says the opposite: the warning at fault_manager_node.cpp:111 reads should be <= 0 (0 or -1 = immediate confirmation). Someone following that log sets 0 and gets the "Invalid debounce thresholds" warning at line 241 instead. Worth changing that string to should be < 0 (-1 = immediate confirmation) in the same PR.
| A reporter that sends one FAILED when a condition appears and one clear when it goes away | ||
| never sends the second event. ``confirmation_threshold: -3`` then leaves the fault in | ||
| PREFAILED, and the default fault list returns CONFIRMED only, so the fault is never seen. | ||
| Healing has the same shape: it needs ``healing_threshold - confirmation_threshold`` |
Collaborator
There was a problem hiding this comment.
The parameter table above (line 68) still says healing_threshold is "Number of PASSED events to transition from CONFIRMED to HEALED", which with the defaults -1/3 is four events, as this box says. README.md:68 already has the counter wording; the table should match.
| healing_threshold: 0 # heal on the single PASSED | ||
|
|
||
| ``auto_confirm_after_sec`` promotes a fault that stayed PREFAILED for that long and looks | ||
| like it would allow a deeper threshold. It does not: HEALED is latched, leaving that latch |
Collaborator
There was a problem hiding this comment.
The "Complete Example" at the bottom of this page (lines 627-630) pairs confirmation_threshold: -3, healing_threshold: 3 and auto_confirm_after_sec: 30.0, which is exactly the combination this box warns about. Either drop auto_confirm_after_sec from the example or note it is for a repeating reporter.
… half Review of the previous commit found the timer path publishing a confirmation that the report path would have suppressed. The report path wraps every EVENT_CONFIRMED publish in `if (!should_mute)`; the timer published unconditionally, so a symptom muted by a root cause reached SSE, the trigger subscribers and the entity freeze frames while the fault list hid it. The timer now asks the correlation engine, through a new `is_muted`. Capture stays ungated, matching the report path, where `just_confirmed` is set regardless of muting. `std::isfinite` was added without `<cmath>`, so it compiled only through a transitive include. The same NaN-through-a-range-check the last commit fixed on `auto_confirm_after_sec` was still present on five other double parameters in this file, two of which feed `create_wall_timer` through a cast; they now share one guard shape and one explanation. `auto_confirm_after_sec` also gains an upper bound: the SQLite backend evaluates the window as `static_cast<int64_t>(value * 1e9)`, undefined once the product leaves int64. `capture_on_confirm` was declared between `publish_fault_event`'s doc comment and its signature, so three `@param` tags bound to the wrong function. The locking comment the extraction left behind moved to the function that takes the lock, and now names both callers. The startup line reports `healing_threshold`, the value this whole area turns on. The warning for a positive `confirmation_threshold` no longer advertises 0, which the sanitizer rejects. `config/fault_manager.yaml` had the same unreachable pairing the bringup preset just lost, under a comment inviting the reader to enable it. Tests: the timer's capture half had no coverage - deleting the call left both suites green. A snapshot case closes that. Its first version did not discriminate either: `GetSnapshots` answers success with an empty topics map when nothing was captured, so the assertion now inspects the payload. The visibility suite waits for the event publisher to match before asserting an absence, measures elapsed time instead of trusting fixed windows, and asserts the event's fault rather than the code it filtered on. `keep_alive` moved below `parametrize`, where the marker survives. Documentation: the complete example no longer ships the pairing the same page warns about, the healing threshold is described as the counter target it is, and five statements that were wrong before this branch are corrected - occurrence counting, the namespaced-deployment example's node nesting, what `audit_log.transitions: all` covers, what happens to out-of-range thresholds, and the claim that PREFAILED implies a negative counter.
Subscription.get_publisher_count does not exist in rclpy on every supported distro, so the wait for the event publisher to match raised AttributeError on Humble before any test ran. Node.count_publishers answers the same question and is present across all of them. The topic name is now a constant, so the subscription and the count cannot drift apart.
The gate that stops a timer confirmation announcing a muted symptom had no test. No integration test set a correlation config together with auto_confirm_after_sec, so the path was unproven either way. The new case reports a root cause and then a symptom inside the rule window, and asserts three things: the root cause is announced, the symptom is not, and the symptom is still CONFIRMED in the store when muted faults are included. The third assertion is what makes the other two mean something. Without it the case would also pass if the timer had never confirmed the symptom at all, which is a different behaviour with the same visible result. Reuses the hierarchical rule already in test_correlation.yaml, so no new configuration file. Removing the gate fails the case and prints the symptom's own fault_confirmed event.
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.
Pull Request
Summary
The debounce counter only moves when an event arrives, so
confirmation_thresholdandhealing_thresholdare tunable only for a reporter that keeps sending events while a conditionholds. For a reporter that sends one FAILED when a condition appears and one clear when it goes
away, the second event never comes: confirmation stalls in PREFAILED, and healing needs
healing_threshold - confirmation_thresholdconsecutive PASSED events when only one is sent. Thedocs recommended
-3withhealing_threshold: 3to everyone, with no mention of this.Writing that down turned up four real defects on the same surface, all fixed here.
A time-based confirmation was invisible. The auto-confirm timer only wrote to the database and
the audit log. The report path also publishes
EVENT_CONFIRMEDand enqueues snapshot + rosbagcapture, so the same fault produced an event and a recording when confirmed by a report, and
neither when confirmed by the timer. Anything driven off the event stream saw no alarm. The capture
block is now a helper shared by both paths so they cannot drift apart again.
auto_confirm_after_secaccepted NaN. Every comparison against NaN is false, so the< 0.0guard let it through and the
> 0.0timer guard then rejected it, disabling time-basedconfirmation with nothing logged at all. A negative value at least warned.
The startup line reported the requested thresholds, not the effective ones. A value the
sanitizer replaced was logged as if it had been accepted, so the log described a configuration that
was not running. Related: the configuration guide recommended
confirmation_threshold: 0, whichthe node rejects and replaces with
-1.bringup_params.yamlnever healed. It pairedhealing_enabled: truewithhealing_threshold: 3under a comment promising a fault heals when its action recovers. The actionbridge emits exactly one PASSED per recovery, and its own README already says healing needs
threshold 0.
Issue
Type
Testing
Two integration tests, both driving the real node over the real services with SQLite storage and
the event counts a one-event-per-transition reporter actually sends.
test_auto_confirm_visibilitysubscribes to the event topic and asserts a timer-drivenconfirmation publishes
EVENT_CONFIRMED. Reverting the fix fails it with the defect in the message:test_debounce_and_healingis parametrized overhealing_thresholdso that 0 is shown to be whatmakes healing reachable rather than asserted to be. The run at the default of 3 asserts the fault
latches instead of skipping, and a re-raise case pins that a healed fault confirms again when the
condition returns. Setting
healing_enabled: falsefails three cases in the[0]variant and nonein
[3], which is where the discriminating power should sit.The NaN and startup-log fixes were checked by running the node at
.nan, at-1.0and atconfirmation_threshold: 0, before and after.Package suite: 761 tests, 0 errors, 0 failures, 37 skipped. Build clean, no warnings.
Checklist