Correct provisional action-fault attribution via supersedes_source_id#536
Open
bburda wants to merge 3 commits into
Open
Correct provisional action-fault attribution via supersedes_source_id#536bburda wants to merge 3 commits into
bburda wants to merge 3 commits into
Conversation
An action fault raised before DDS discovery resolves the server node FQN was
reported under the action interface name (e.g. /navigate_to_pose). The
strict-AND per-entity /faults scope filter then kept that fault hidden from
every node entity, because the provisional source stayed in the append-only
reporting_sources set permanently.
Add ReportFault.supersedes_source_id: a re-report can drop a previously
reported source before adding the new one. The action bridge records the
provisional source and, once discovery resolves the FQN, re-reports the
already-delivered fault under the server FQN with the provisional source
superseded, so the FaultManager ends with reporting_sources = {FQN} and the
fault resolves to the server node's entity.
- ReportFault.srv: optional supersedes_source_id field (empty = no supersede);
out-of-tree callers must rebuild against the new message
- FaultStorage / SqliteFaultStorage: drop the superseded source before adding
the new one, in both the FAILED-update and CLEARED-reactivation paths;
a self-supersede is a no-op
- FaultReporter::report gains an optional supersedes_source_id; a supersede
bypasses local filtering so the correcting report is never debounced away
- action_status_bridge: track the provisional source per action and correct it
on the next rescan once the FQN is known; a healed or not-yet-delivered fault
is left untouched
- Unit tests for every storage path and the bridge re-attribution decisions
- End-to-end test driving the real gateway scope filter and FaultManager
- Docs and CHANGELOGs
Closes #467
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a fault-attribution edge case where action faults raised before DDS discovery resolves the server node FQN could remain permanently scoped under a provisional source, preventing them from appearing under the correct entity due to strict-AND fault scope filtering.
Changes:
- Adds
ReportFault.supersedes_source_idto allow replacing a previously reported source inreporting_sources(opt-in; default preserves add-only behavior). - Implements supersede handling in both in-memory and SQLite fault storage, and exposes the new parameter via
FaultReporter::report(bypassing local debounce when superseding). - Updates
action_status_bridgeto track provisional sources and re-report once the server FQN is resolved; adds unit + integration test coverage and updates docs/changelogs.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_msgs/srv/ReportFault.srv | Adds supersedes_source_id request field and documents intended semantics. |
| src/ros2_medkit_msgs/README.md | Documents the new supersedes_source_id field for ReportFault. |
| src/ros2_medkit_msgs/CHANGELOG.rst | Notes the new service field and rebuild requirement for out-of-tree users. |
| src/ros2_medkit_integration_tests/test/features/test_fault_source_supersede.test.py | Adds an end-to-end test validating provisional-source supersede fixes strict-AND scoping. |
| src/ros2_medkit_fault_reporter/src/fault_reporter.cpp | Extends reporter API to forward supersede and bypass local debounce when superseding. |
| src/ros2_medkit_fault_reporter/include/ros2_medkit_fault_reporter/fault_reporter.hpp | Updates reporter API documentation/signature to include optional supersede argument. |
| src/ros2_medkit_fault_reporter/README.md | Documents advanced reporter usage for superseding provisional sources. |
| src/ros2_medkit_fault_reporter/CHANGELOG.rst | Changelog entry for the reporter API extension and debounce bypass behavior. |
| src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp | Extends storage interface contract with optional supersede parameter. |
| src/ros2_medkit_fault_manager/src/fault_storage.cpp | Implements supersede behavior in the in-memory store (self-supersede guarded). |
| src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/sqlite_fault_storage.hpp | Extends SQLite storage override signature with optional supersede parameter. |
| src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp | Implements supersede behavior in SQLite storage by updating stored source set. |
| src/ros2_medkit_fault_manager/src/fault_manager_node.cpp | Forwards supersedes_source_id from service request to storage. |
| src/ros2_medkit_fault_manager/test/test_fault_manager.cpp | Adds unit tests covering supersede behavior in the in-memory store. |
| src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp | Adds unit tests covering supersede behavior persisted through SQLite JSON storage. |
| src/ros2_medkit_fault_manager/CHANGELOG.rst | Changelog entry for supersede behavior in fault storage implementations. |
| src/ros2_medkit_action_status_bridge/include/ros2_medkit_action_status_bridge/action_status_bridge_node.hpp | Adds re-attribution support structures/API and test accessors. |
| src/ros2_medkit_action_status_bridge/src/action_status_bridge_node.cpp | Implements provisional tracking + re-attribution on rescan; cleans up state on prune. |
| src/ros2_medkit_action_status_bridge/test/test_action_status_bridge.cpp | Adds deterministic unit tests for re-attribution behavior via an overridable resolver seam. |
| src/ros2_medkit_action_status_bridge/README.md | Updates behavior description to include provisional re-attribution via supersede. |
| src/ros2_medkit_action_status_bridge/CHANGELOG.rst | Changelog entry for corrected action-fault attribution behavior. |
| docs/api/messages.rst | Updates API docs to include supersedes_source_id semantics and usage notes. |
Comment on lines
+498
to
+501
| void ActionStatusBridgeNode::reattribute_provisional() { | ||
| // Snapshot the provisional action names, then process each without holding a lock | ||
| // across the per-action state read + graph query + report. | ||
| std::vector<std::string> actions; |
The gateway-scope e2e proves the supersede mechanism but hand-fires the ReportFault calls; the existing bridge integration test deliberately synchronizes discovery to avoid the race. Neither drives the real bridge through the abort-before-discovery path that #467 is about. Add test_reattribution_e2e.test.py: it runs the real ActionStatusBridgeNode against a real FaultManager, substituting only the one inherently racy primitive - resolving the server node FQN from DDS discovery - via a test-double executable (slow_discovery_bridge) that forces the first few resolutions to "" (as rcl does before a participant's node name propagates). This reproduces the race deterministically: the fault first surfaces under the provisional action-name source, then reattribute_provisional() re-reports under the resolved FQN with supersedes_source_id set, so reporting_sources ends as {FQN} with the provisional source dropped. The test-double overrides the private virtual server_fqn_for_action (a Non-Virtual-Interface customization point) and resolves the FQN for real via the public graph API; the production bridge is unchanged.
- test_fault_source_supersede: read reporting_sources from the x-medkit extension block, not the SOVD "item" block (item carries only code/name/severity/status). The post-supersede assertion was always comparing against an empty list. - test_fault_source_supersede: add the #!/usr/bin/env python3 shebang the other integration tests use. - test_reattribution_e2e: move the module docstring summary to the second line to satisfy pep257 D213. - reattribute_provisional: correct the locking comment - the graph query and re-report run under reporters_mutex_ (no deadlock: neither server_fqn_for_action nor FaultReporter::report re-acquires it).
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
An action fault raised before DDS discovery resolves the action server node's
FQN was reported under the action interface name (e.g.
/navigate_to_pose).Because
reporting_sourcesis an append-only set and the per-entity/faultsscope filter is strict-AND (every source must be in the entity's scope), that
provisional source stayed in the set permanently and kept the fault hidden from
every node entity, including the real server node it belongs to.
This adds an optional
ReportFault.supersedes_source_id: a re-report can drop apreviously-reported source before adding the new one. The
action_status_bridgerecords the provisional source and, once discovery resolves the FQN, re-reports
the already-delivered fault under the server FQN with the provisional source
superseded, so the FaultManager ends with
reporting_sources = {FQN}and thefault resolves to the correct entity.
Behavior is opt-in and backward compatible: an empty
supersedes_source_id(the default) preserves the existing add-only behavior.
Key pieces:
ReportFault.srv: optionalsupersedes_source_idfield.FaultStorage/SqliteFaultStorage: drop the superseded source beforeadding the new one, in both the FAILED-update and CLEARED-reactivation paths;
a self-supersede (
supersedes == source_id) is a no-op so a fault can neverend up with an empty source set.
FaultReporter::report: optionalsupersedes_source_id; a supersedebypasses local debounce filtering so the correcting report is never dropped.
action_status_bridge: tracks the provisional source per action andcorrects it on the next rescan once the FQN is known. A healed or
not-yet-delivered fault is left untouched; re-attribution happens at most once
per fault.
Issue
Type
Note: not a breaking change (the new field/argument default to empty), but
out-of-tree callers must rebuild against the regenerated
ros2_medkit_msgs.Testing
self-supersede guard, supersede on a brand-new fault, and supersede across a
CLEARED->FAILED reactivation - for both the in-memory and SQLite stores.
resolved once delivered, and kept after a heal - driven deterministically
through a test seam with a controllable server-FQN resolver.
test_fault_source_supersede.test.pydrives the real gatewayscope filter and FaultManager - a fault reported under a provisional source is
404 under the owning app, then 200 after a superseding re-report under the FQN.
This exercises the exact strict-AND scope-filter interaction that caused Re-attribute action faults raised before DDS discovery resolves #467.
The bridge's discovery-race timing itself cannot be reproduced deterministically
in a launch test, so the bridge decision logic is covered by the unit seam and
the gateway/FaultManager half by the e2e; together they cover the full path.
Local runs (Jazzy): fault_manager 457, fault_reporter 47, action_status_bridge
65 unit tests - 0 failures; clang-format, clang-tidy (changed files), flake8,
and ament-copyright all clean.
Checklist