From 80bd34663e0b544a9480856da45c16f469a4b1c5 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 16 Jul 2026 09:11:23 +0200 Subject: [PATCH 1/3] Correct provisional action-fault attribution via supersedes_source_id 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 --- docs/api/messages.rst | 8 + .../CHANGELOG.rst | 5 + .../README.md | 8 +- .../action_status_bridge_node.hpp | 44 ++++- .../src/action_status_bridge_node.cpp | 126 ++++++++++++- .../test/test_action_status_bridge.cpp | 96 +++++++++- src/ros2_medkit_fault_manager/CHANGELOG.rst | 1 + .../fault_storage.hpp | 8 +- .../sqlite_fault_storage.hpp | 3 +- .../src/fault_manager_node.cpp | 5 +- .../src/fault_storage.cpp | 19 +- .../src/sqlite_fault_storage.cpp | 10 +- .../test/test_fault_manager.cpp | 76 ++++++++ .../test/test_sqlite_storage.cpp | 51 +++++ src/ros2_medkit_fault_reporter/CHANGELOG.rst | 5 + src/ros2_medkit_fault_reporter/README.md | 16 ++ .../fault_reporter.hpp | 9 +- .../src/fault_reporter.cpp | 15 +- .../test_fault_source_supersede.test.py | 174 ++++++++++++++++++ src/ros2_medkit_msgs/CHANGELOG.rst | 5 + src/ros2_medkit_msgs/README.md | 1 + src/ros2_medkit_msgs/srv/ReportFault.srv | 8 + 22 files changed, 663 insertions(+), 30 deletions(-) create mode 100644 src/ros2_medkit_integration_tests/test/features/test_fault_source_supersede.test.py diff --git a/docs/api/messages.rst b/docs/api/messages.rst index 4ecd08e30..b5bee239c 100644 --- a/docs/api/messages.rst +++ b/docs/api/messages.rst @@ -179,6 +179,7 @@ Report a fault event to the FaultManager. uint8 severity # Fault.SEVERITY_* constant (for FAILED events) string description # Human-readable description string source_id # Fully qualified node name (e.g., "/powertrain/temp_sensor") + string supersedes_source_id # Optional: a prior source_id to drop before source_id is added **Response:** @@ -186,6 +187,13 @@ Report a fault event to the FaultManager. bool accepted # True if event was accepted +Setting ``supersedes_source_id`` drops that source from the fault's +``reporting_sources`` before ``source_id`` is added, letting a reporter correct a +provisional attribution (for example an action bridge that first reports under the +action interface name, then re-reports under the resolved server node FQN once DDS +discovery settles). Empty (default) leaves the append-only behavior unchanged. Only +meaningful for a FAILED event on an existing fault; a self-supersede is a no-op. + **Example Usage:** .. code-block:: cpp diff --git a/src/ros2_medkit_action_status_bridge/CHANGELOG.rst b/src/ros2_medkit_action_status_bridge/CHANGELOG.rst index 861ebc152..a508a7dc2 100644 --- a/src/ros2_medkit_action_status_bridge/CHANGELOG.rst +++ b/src/ros2_medkit_action_status_bridge/CHANGELOG.rst @@ -2,6 +2,11 @@ Changelog for package ros2_medkit_action_status_bridge ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Forthcoming +----------- +* A fault first raised before DDS discovery resolves the action server node is now corrected once the FQN becomes known, instead of staying stuck under the provisional action-interface source. The bridge records the provisional source, and on the next rescan re-reports the already-delivered fault under the resolved server FQN with ``ReportFault.supersedes_source_id`` set, so the FaultManager drops the provisional source and the fault resolves to the server node's entity under the strict-AND per-entity ``/faults`` scope filter. This refines the #466 "source fixed at first report" behavior: the source is still fixed for a fault whose server was already known, but a provisional attribution made before discovery is corrected rather than left permanently mis-scoped. A healed or not-yet-delivered fault is left untouched (`#467 `_) +* Contributors: @bburda + 0.6.0 (2026-06-22) ------------------ * Initial release: generic action-status bridge. Watches every diff --git a/src/ros2_medkit_action_status_bridge/README.md b/src/ros2_medkit_action_status_bridge/README.md index 57cd30662..0d566be94 100644 --- a/src/ros2_medkit_action_status_bridge/README.md +++ b/src/ros2_medkit_action_status_bridge/README.md @@ -32,7 +32,13 @@ generically, by observing the goal-status topic. `ACTION_NAVIGATE_TO_POSE_ABORTED`. `source_id` is the action **server's node FQN** (resolved from the status topic's publisher, e.g. `/bt_navigator`), so the fault associates with that node's SOVD entity; it falls back to the action - name only if no publisher is visible. + name only if no publisher is visible. When the FQN is not yet discovered at the + first report, the fault fires on time under the action-name fallback and is + then **re-attributed** to the FQN once discovery resolves - the bridge + re-reports it with `ReportFault.supersedes_source_id` set to the fallback, so + the FaultManager drops the provisional source and the fault lands on the server + node's entity (the fallback source alone would keep it out under the strict-AND + per-entity scope filter). ## Per-action state, not per-goal diff --git a/src/ros2_medkit_action_status_bridge/include/ros2_medkit_action_status_bridge/action_status_bridge_node.hpp b/src/ros2_medkit_action_status_bridge/include/ros2_medkit_action_status_bridge/action_status_bridge_node.hpp index 64585aa45..36468cd58 100644 --- a/src/ros2_medkit_action_status_bridge/include/ros2_medkit_action_status_bridge/action_status_bridge_node.hpp +++ b/src/ros2_medkit_action_status_bridge/include/ros2_medkit_action_status_bridge/action_status_bridge_node.hpp @@ -139,8 +139,17 @@ class ActionStatusBridgeNode : public rclcpp::Node { /// Resolve the action server's node FQN from its status-topic publisher, for /// use as the fault source_id so faults associate with the gateway's SOVD /// entity. Returns "" if no publisher with a discovery-resolved node name is - /// visible yet (the caller falls back and re-resolves later). - std::string server_fqn_for_action(const std::string & action_name); + /// visible yet (the caller falls back and re-resolves later). virtual so a unit + /// test can drive the re-attribution path without a live action graph. + virtual std::string server_fqn_for_action(const std::string & action_name); + + /// Re-attribute faults first reported under a provisional (action-name) source + /// to the resolved server FQN. For each provisional action still reported failed, + /// once the FQN resolves it re-reports the fault under the FQN with + /// supersedes_source_id set to the provisional source, so the FaultManager swaps + /// the source and the fault resolves to the server node's SOVD entity. A no-op + /// once every provisional attribution has been corrected. Runs on the rescan tick. + void reattribute_provisional(); /// Returns true if this (goal, status) pair was not logged before, marking it /// logged. Bounded to avoid unbounded growth. Suppresses duplicate LOG lines @@ -172,6 +181,17 @@ class ActionStatusBridgeNode : public rclcpp::Node { std::map> reporters_; std::mutex reporters_mutex_; + // Actions whose fault was first reported under a provisional (action-name) source + // because the server node FQN was not yet discovered. reattribute_provisional() + // corrects each to the FQN once discovery resolves. Guarded by reporters_mutex_. + struct Provisional { + std::string old_source; // the provisional source_id to supersede + // The FQN-sourced reporter, created once the FQN resolves and reused across + // rescan ticks until its service client is ready (avoids re-discovery churn). + std::unique_ptr fqn_reporter; + }; + std::map provisional_; + // Desired (latest observed) action-level state derived from status messages: // the source of truth the bridge tries to make the FaultManager reflect. // `canceled` records whether a failure was a CANCELED (vs ABORTED), to pick @@ -187,6 +207,10 @@ class ActionStatusBridgeNode : public rclcpp::Node { // so reconcile_pending() retries it on the next rescan instead of dropping it. std::map desired_state_; std::map last_reported_state_; + // The `canceled` flag as of the report that put each action into kFailed, so a + // deferred re-attribution supersedes the fault code that was actually reported + // (not a value that flipped afterwards). Guarded by state_mutex_. + std::map reported_canceled_; std::mutex state_mutex_; // Bounded dedup of logged (goal_id:status) keys (LOG suppression only). @@ -251,6 +275,22 @@ class ActionStatusBridgeTestAccess { /// test assert the reporter is created once and never swapped out. const void * reporter_identity(const std::string & action_name); + /// Run the re-attribution pass (what the rescan tick fires) so a test can drive + /// the provisional -> FQN correction without a live rescan timer. + void run_reattribute_provisional(); + + /// True if the action currently has a provisional (action-name) attribution + /// pending correction to the server FQN. + bool is_provisional(const std::string & action_name) const; + + /// True if re-attribution has resolved the FQN and created (but not yet + /// delivered, e.g. service not ready) the FQN-sourced reporter for this action. + bool provisional_has_fqn_reporter(const std::string & action_name) const; + + /// The `canceled` flag captured for the action's currently-reported fault + /// (what a deferred re-attribution will supersede under). + bool reported_canceled(const std::string & action_name) const; + private: ActionStatusBridgeNode * node_; }; diff --git a/src/ros2_medkit_action_status_bridge/src/action_status_bridge_node.cpp b/src/ros2_medkit_action_status_bridge/src/action_status_bridge_node.cpp index 7d64431f6..34cda0c20 100644 --- a/src/ros2_medkit_action_status_bridge/src/action_status_bridge_node.cpp +++ b/src/ros2_medkit_action_status_bridge/src/action_status_bridge_node.cpp @@ -214,6 +214,10 @@ void ActionStatusBridgeNode::rescan_actions() { // arming can never strand a pending report. The fast timer is what keeps the // FaultManager freeze-frame contemporaneous; this is just belt-and-suspenders. reconcile_pending(); + + // Correct any faults reported under a provisional (action-name) source now that + // discovery has had another rescan period to resolve the server FQN. + reattribute_provisional(); } void ActionStatusBridgeNode::prune_vanished(const std::map & present_topics) { @@ -257,11 +261,13 @@ void ActionStatusBridgeNode::prune_vanished(const std::map lock(state_mutex_); last_reported_state_.erase(action_name); desired_state_.erase(action_name); + reported_canceled_.erase(action_name); } RCLCPP_INFO(get_logger(), "Action '%s' vanished; dropped (topic %s)", action_name.c_str(), topic.c_str()); it = subs_.erase(it); @@ -358,6 +364,9 @@ ActionStatusBridgeNode::reconcile(const std::string & action_name, RCLCPP_INFO(get_logger(), "Action %s -> fault %s", action_name.c_str(), code.c_str()); } last_reported_state_[action_name] = ActionState::kFailed; + // Remember which code (aborted vs canceled) was actually raised, so a later + // re-attribution supersedes that exact fault, not one derived from a flipped flag. + reported_canceled_[action_name] = desired.canceled; return ActionState::kFailed; } @@ -462,7 +471,7 @@ ros2_medkit_fault_reporter::FaultReporter * ActionStatusBridgeNode::reporter_for std::lock_guard lock(reporters_mutex_); auto it = reporters_.find(action_name); if (it != reporters_.end()) { - return it->second.get(); // created on the first report; source is fixed thereafter + return it->second.get(); // created on the first report; source fixed until re-attributed } // First report for this action: attribute it to the action SERVER's node FQN so // the gateway can resolve the fault to its SOVD entity. During DDS discovery the @@ -470,19 +479,103 @@ ros2_medkit_fault_reporter::FaultReporter * ActionStatusBridgeNode::reporter_for // so the fault still fires on time. Reporting the fault on time takes priority // over entity attribution when discovery is slow. // - // The source is NOT re-attributed later: reporting_sources is append-only on the - // manager side and the per-entity /faults scope filter is strict-AND, so a - // provisional action-name source cannot be swapped for the FQN afterwards. - // Correct attribution for the slow-discovery case is a separate concern (it - // needs a way to supersede a provisional source). + // A fallback (action-name) attribution is recorded as provisional and later + // corrected to the FQN by reattribute_provisional(): reporting_sources is + // append-only and the per-entity /faults scope filter is strict-AND, so the + // provisional source cannot merely be added-to - it is swapped for the FQN via + // ReportFault.supersedes_source_id once discovery resolves. const std::string fqn = server_fqn_for_action(action_name); const std::string & source_id = fqn.empty() ? action_name : fqn; + if (fqn.empty()) { + provisional_[action_name] = Provisional{action_name, nullptr}; + } auto reporter = std::make_unique(this->shared_from_this(), source_id); auto * raw = reporter.get(); reporters_[action_name] = std::move(reporter); return raw; } +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 actions; + { + std::lock_guard lock(reporters_mutex_); + actions.reserve(provisional_.size()); + for (const auto & [action_name, prov] : provisional_) { + actions.push_back(action_name); + } + } + if (actions.empty()) { + return; + } + + for (const auto & action_name : actions) { + // Re-attribution applies only to a fault that is actually LIVE in the + // FaultManager under the provisional source. Read the delivered state: + // - not delivered yet (no last_reported entry): the fault does not exist yet + // (its FAILED is still deferred on service discovery). Keep the entry and + // wait - erasing here would strand the report so it later lands under the + // provisional source with nothing left to correct it. + // - healed (kHealthy): nothing active to correct now, but a re-failure would + // reactivate under the still-provisional source (reporter_for reuses the + // frozen reporter), so keep the entry to correct it then. + // - failed (kFailed): correct it. + // reported_canceled_ carries the canceled flag AS OF the report that raised the + // fault, so the supersede targets the code actually reported, not a later flip. + bool has_reported = false; + ActionState reported = ActionState::kUnknown; + bool reported_canceled = false; + { + std::lock_guard lock(state_mutex_); + auto sit = last_reported_state_.find(action_name); + has_reported = sit != last_reported_state_.end(); + if (has_reported) { + reported = sit->second; + } + auto cit = reported_canceled_.find(action_name); + reported_canceled = cit != reported_canceled_.end() && cit->second; + } + + std::lock_guard lock(reporters_mutex_); + auto pit = provisional_.find(action_name); + if (pit == provisional_.end()) { + continue; // raced away (e.g. pruned) + } + if (!has_reported || reported != ActionState::kFailed) { + continue; // not delivered yet, or healed: keep the entry, nothing to correct now + } + + // Resolve the server FQN once, then keep the FQN reporter across ticks until + // its service client is ready (recreating it every tick would reset discovery). + if (!pit->second.fqn_reporter) { + const std::string fqn = server_fqn_for_action(action_name); + if (fqn.empty() || fqn == pit->second.old_source) { + continue; // still unresolved; retry on the next rescan + } + pit->second.fqn_reporter = + std::make_unique(this->shared_from_this(), fqn); + } + if (!pit->second.fqn_reporter->is_service_ready()) { + continue; // FaultManager service not discovered by the new client yet; retry + } + + // Re-report the same fault (the code actually reported, per reported_canceled_) + // under the FQN, superseding the provisional source, so the FaultManager ends + // with reporting_sources = {FQN} and the fault resolves to the server node's + // SOVD entity under the strict-AND scope filter. This second FAILED report bumps + // the fault's occurrence_count by one; the correct scope attribution is worth that + // cosmetic increment, and re-attribution happens at most once per fault. + const std::string code = fault_code_for(action_name, reported_canceled); + const std::string desc = std::string("Action ") + action_name + (reported_canceled ? " canceled" : " aborted"); + pit->second.fqn_reporter->report(code, aborted_severity_, desc, pit->second.old_source); + RCLCPP_INFO(get_logger(), "Re-attributed action '%s' fault '%s' from provisional '%s' to server FQN", + action_name.c_str(), code.c_str(), pit->second.old_source.c_str()); + reporters_[action_name] = std::move(pit->second.fqn_reporter); + provisional_.erase(pit); + } +} + std::string ActionStatusBridgeNode::server_fqn_from_endpoint(const std::string & node_name, const std::string & node_namespace) { // During DDS discovery the participant is known before its node name/namespace @@ -670,4 +763,25 @@ const void * ActionStatusBridgeTestAccess::reporter_identity(const std::string & return node_->reporter_for(action_name); } +void ActionStatusBridgeTestAccess::run_reattribute_provisional() { + node_->reattribute_provisional(); +} + +bool ActionStatusBridgeTestAccess::is_provisional(const std::string & action_name) const { + std::lock_guard lock(node_->reporters_mutex_); + return node_->provisional_.count(action_name) > 0; +} + +bool ActionStatusBridgeTestAccess::provisional_has_fqn_reporter(const std::string & action_name) const { + std::lock_guard lock(node_->reporters_mutex_); + auto it = node_->provisional_.find(action_name); + return it != node_->provisional_.end() && it->second.fqn_reporter != nullptr; +} + +bool ActionStatusBridgeTestAccess::reported_canceled(const std::string & action_name) const { + std::lock_guard lock(node_->state_mutex_); + auto it = node_->reported_canceled_.find(action_name); + return it != node_->reported_canceled_.end() && it->second; +} + } // namespace ros2_medkit_action_status_bridge diff --git a/src/ros2_medkit_action_status_bridge/test/test_action_status_bridge.cpp b/src/ros2_medkit_action_status_bridge/test/test_action_status_bridge.cpp index 7bf4aa249..c9f13bd40 100644 --- a/src/ros2_medkit_action_status_bridge/test/test_action_status_bridge.cpp +++ b/src/ros2_medkit_action_status_bridge/test/test_action_status_bridge.cpp @@ -278,17 +278,107 @@ TEST_F(ActionStatusBridgeTest, Apply_PerActionIsolation) { // --- reporter stickiness: created once, source fixed, never swapped --- -TEST_F(ActionStatusBridgeTest, ReporterFor_StickyCreatedOnceNeverSwapped) { +TEST_F(ActionStatusBridgeTest, ReporterFor_StickyCreatedOncePerCall) { auto node = std::make_shared(); ActionStatusBridgeTestAccess access(node.get()); // No publisher exists, so the server FQN is unresolved and the reporter falls - // back to the action name. It must be created once and reused, never swapped: - // reporting_sources is append-only, so a provisional source cannot be undone. + // back to the action name. reporter_for() itself creates the reporter once and + // returns the same instance on repeat calls (it never swaps the source on its + // own). Correction to the FQN happens separately, in reattribute_provisional(). const void * first = access.reporter_identity("/nav"); EXPECT_NE(first, nullptr); EXPECT_EQ(access.reporter_identity("/nav"), first); } +// A bridge whose server-FQN resolution is test-controlled, so re-attribution +// (issue #467) can be driven deterministically without a live DDS action graph. +class FqnControllableBridge : public ActionStatusBridgeNode { + public: + std::string forced_fqn; // "" = FQN still unresolved (report falls back to the action name) + + private: + std::string server_fqn_for_action(const std::string & /*action_name*/) override { + return forced_fqn; + } +}; + +// Regression (#467, HIGH): a provisional fault whose FAILED report is still DEFERRED +// (FaultManager service not discovered) must NOT be dropped by re-attribution - +// otherwise the later-delivered report is stranded under the action-name source with +// nothing left to correct it. reattribute must keep the entry until the fault is live. +TEST_F(ActionStatusBridgeTest, Reattribute_KeepsProvisionalWhileReportDeferred) { + auto node = std::make_shared(); + ActionStatusBridgeTestAccess access(node.get()); + node->forced_fqn = ""; // unresolved at first report -> provisional fallback + + auto * reporter = access.reporter_for("/nav"); + ASSERT_FALSE(reporter->is_service_ready()); // no FaultManager in this unit test + ASSERT_TRUE(access.is_provisional("/nav")); + + // ABORTED whose report cannot be delivered: pending, not delivered. + EXPECT_EQ(node->apply_message("/nav", one_goal(1, GoalStatus::STATUS_ABORTED), reporter), State::kUnknown); + ASSERT_FALSE(access.reported_failed("/nav")); + ASSERT_TRUE(access.pending_failed("/nav")); + + // FQN is now resolvable, but the fault does not exist in the manager yet. + node->forced_fqn = "/bt_navigator"; + access.run_reattribute_provisional(); + + EXPECT_TRUE(access.is_provisional("/nav")); // kept, not stranded + EXPECT_FALSE(access.provisional_has_fqn_reporter("/nav")); // not corrected: nothing delivered +} + +// Once the fault is delivered AND the FQN resolves, re-attribution creates the FQN +// reporter (reused across ticks until its service is ready, no discovery churn). It +// cannot deliver the supersede here (no FaultManager service), so the entry is kept +// with the FQN reporter stashed; the supersede itself is proven by the storage unit +// tests and the fault-source-supersede integration test. +TEST_F(ActionStatusBridgeTest, Reattribute_ResolvesFqnOnceDelivered) { + auto node = std::make_shared(); + ActionStatusBridgeTestAccess access(node.get()); + node->forced_fqn = ""; + + ASSERT_NE(access.reporter_for("/nav"), nullptr); // provisional recorded (FQN unresolved) + ASSERT_TRUE(access.is_provisional("/nav")); + // Deliver the ABORTED (null-reporter seam stands in for a discovered service). + EXPECT_EQ(node->apply_message("/nav", one_goal(1, GoalStatus::STATUS_ABORTED), nullptr), State::kFailed); + ASSERT_TRUE(access.reported_failed("/nav")); + EXPECT_FALSE(access.reported_canceled("/nav")); // aborted, not canceled -> superseded as _ABORTED + + // FQN still unresolved -> cannot correct yet; entry kept, no FQN reporter. + access.run_reattribute_provisional(); + EXPECT_TRUE(access.is_provisional("/nav")); + EXPECT_FALSE(access.provisional_has_fqn_reporter("/nav")); + + // FQN resolves -> re-attribution resolves it and stashes the FQN reporter. + node->forced_fqn = "/bt_navigator"; + access.run_reattribute_provisional(); + EXPECT_TRUE(access.is_provisional("/nav")); + EXPECT_TRUE(access.provisional_has_fqn_reporter("/nav")); +} + +// Regression (#467, HIGH): re-attribution must NOT drop the provisional entry when +// the fault heals - a later re-failure reactivates under the still-provisional source +// (reporter_for reuses the frozen reporter) and must be correctable then. +TEST_F(ActionStatusBridgeTest, Reattribute_KeepsProvisionalAfterHeal) { + auto node = std::make_shared(); + ActionStatusBridgeTestAccess access(node.get()); + node->forced_fqn = ""; + + ASSERT_NE(access.reporter_for("/nav"), nullptr); + ASSERT_TRUE(access.is_provisional("/nav")); + // Raise then heal, both delivered (null seam). + EXPECT_EQ(node->apply_message("/nav", one_goal(1, GoalStatus::STATUS_ABORTED), nullptr), State::kFailed); + EXPECT_EQ(node->apply_message("/nav", one_goal(2, GoalStatus::STATUS_SUCCEEDED), nullptr), State::kHealthy); + ASSERT_FALSE(access.reported_failed("/nav")); + + // Re-attribution on a healed fault must keep the entry (even with the FQN now + // resolvable): a re-failure would need correcting, and the old code erased it here. + node->forced_fqn = "/bt_navigator"; + access.run_reattribute_provisional(); + EXPECT_TRUE(access.is_provisional("/nav")); +} + // --- deferred delivery: a fault that cannot reach the FaultManager yet (its // service is not discovered) must be kept pending and retried, never silently // dropped. This is the startup discovery race a latched ABORTED status hits. --- diff --git a/src/ros2_medkit_fault_manager/CHANGELOG.rst b/src/ros2_medkit_fault_manager/CHANGELOG.rst index 1e884c910..08d8b0a4a 100644 --- a/src/ros2_medkit_fault_manager/CHANGELOG.rst +++ b/src/ros2_medkit_fault_manager/CHANGELOG.rst @@ -5,6 +5,7 @@ Changelog for package ros2_medkit_fault_manager Forthcoming ----------- * Optional append-only, hash-chained audit log of fault state transitions: each transition appends one immutable row (``record_hash = sha256(prev_hash + canonical(event))`` via OpenSSL EVP SHA-256) with a persisted chain head, a ``verify`` routine, a read API, and retention that seals a segment anchor before pruning. Time-based (PREFAILED->CONFIRMED) auto-confirmations are also audited. ``verify`` reads the chain head directly from the database, so deleting the newest row together with the head row is reported as tampering instead of silently recovering. ``BEFORE UPDATE`` / ``BEFORE DELETE`` triggers reject out-of-band edits as defense-in-depth. The chain is unkeyed and stored in a single writable file, so ``verify`` detects edits/deletions that did not recompute the chain (casual or accidental tampering); it is not a defence against an attacker who can rewrite the whole file. Off by default (`#483 `_) +* ``report_fault_event`` honors the new ``ReportFault.supersedes_source_id`` request field: when set, the named source is dropped from the fault's ``reporting_sources`` before the current source is added (both the in-memory and SQLite stores). This lets a reporter correct an attribution it made under a provisional source without leaving the stale source stuck in the append-only set, where the strict-AND per-entity ``/faults`` scope filter would keep the fault hidden. Empty (default) preserves the prior add-only behavior; a self-supersede (``supersedes_source_id == source_id``) is a no-op (`#467 `_) 0.6.0 (2026-06-22) ------------------ diff --git a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp index 25f7b0dc0..c04c62cdb 100644 --- a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp +++ b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_storage.hpp @@ -151,11 +151,14 @@ class FaultStorage { /// @param source_id Reporting source identifier /// @param timestamp Current time for tracking /// @param config Debounce configuration to apply for this event (resolved per-entity by the node) + /// @param supersedes_source_id Optional source_id to remove from reporting_sources before adding + /// source_id (empty = no supersede). Lets a reporter correct a provisional attribution. /// @return true if this is a new occurrence (new fault or reactivated CLEARED fault), /// false if existing active fault was updated virtual bool report_fault_event(const std::string & fault_code, uint8_t event_type, uint8_t severity, const std::string & description, const std::string & source_id, - const rclcpp::Time & timestamp, const DebounceConfig & config) = 0; + const rclcpp::Time & timestamp, const DebounceConfig & config, + const std::string & supersedes_source_id = "") = 0; /// Get faults matching filter criteria /// @param filter_by_severity Whether to filter by severity @@ -273,7 +276,8 @@ class InMemoryFaultStorage : public FaultStorage { bool report_fault_event(const std::string & fault_code, uint8_t event_type, uint8_t severity, const std::string & description, const std::string & source_id, - const rclcpp::Time & timestamp, const DebounceConfig & config) override; + const rclcpp::Time & timestamp, const DebounceConfig & config, + const std::string & supersedes_source_id = "") override; std::vector list_faults(bool filter_by_severity, uint8_t severity, const std::vector & statuses) const override; diff --git a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/sqlite_fault_storage.hpp b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/sqlite_fault_storage.hpp index 674e63baf..90e98199e 100644 --- a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/sqlite_fault_storage.hpp +++ b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/sqlite_fault_storage.hpp @@ -46,7 +46,8 @@ class SqliteFaultStorage : public FaultStorage { bool report_fault_event(const std::string & fault_code, uint8_t event_type, uint8_t severity, const std::string & description, const std::string & source_id, - const rclcpp::Time & timestamp, const DebounceConfig & config) override; + const rclcpp::Time & timestamp, const DebounceConfig & config, + const std::string & supersedes_source_id = "") override; std::vector list_faults(bool filter_by_severity, uint8_t severity, const std::vector & statuses) const override; diff --git a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp index 2ced5abb2..aa625a138 100644 --- a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp @@ -607,8 +607,9 @@ void FaultManagerNode::handle_report_fault( // Report the fault event (use wall clock time, not sim time, for proper timestamps) const rclcpp::Time event_time = get_wall_clock_time(); - bool is_new = storage_->report_fault_event(request->fault_code, request->event_type, request->severity, - request->description, request->source_id, event_time, resolved_config); + bool is_new = + storage_->report_fault_event(request->fault_code, request->event_type, request->severity, request->description, + request->source_id, event_time, resolved_config, request->supersedes_source_id); response->accepted = true; diff --git a/src/ros2_medkit_fault_manager/src/fault_storage.cpp b/src/ros2_medkit_fault_manager/src/fault_storage.cpp index 269bb9f93..8f4c6ee49 100644 --- a/src/ros2_medkit_fault_manager/src/fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_storage.cpp @@ -106,11 +106,22 @@ void InMemoryFaultStorage::update_status(FaultState & state, const DebounceConfi bool InMemoryFaultStorage::report_fault_event(const std::string & fault_code, uint8_t event_type, uint8_t severity, const std::string & description, const std::string & source_id, - const rclcpp::Time & timestamp, const DebounceConfig & config) { + const rclcpp::Time & timestamp, const DebounceConfig & config, + const std::string & supersedes_source_id) { std::lock_guard lock(mutex_); const bool is_failed = (event_type == EventType::EVENT_FAILED); + // Drop a provisional source before adding the new one, so a reporter can correct an + // earlier attribution. No-op on a brand-new fault (nothing to remove) or when the + // superseded source equals source_id. Applied in each source-insert path below. + const auto add_source = [&](std::set & sources) { + if (!supersedes_source_id.empty() && supersedes_source_id != source_id) { + sources.erase(supersedes_source_id); + } + sources.insert(source_id); + }; + auto it = faults_.find(fault_code); if (it == faults_.end()) { // New fault - only create entry for FAILED events @@ -127,7 +138,7 @@ bool InMemoryFaultStorage::report_fault_event(const std::string & fault_code, ui state.last_failed_time = timestamp; state.occurrence_count = 1; state.debounce_counter = -1; // First FAILED event - state.reporting_sources.insert(source_id); + add_source(state.reporting_sources); // CRITICAL severity bypasses debounce and confirms immediately if (config.critical_immediate_confirm && severity == ros2_medkit_msgs::msg::Fault::SEVERITY_CRITICAL) { @@ -154,7 +165,7 @@ bool InMemoryFaultStorage::report_fault_event(const std::string & fault_code, ui state.debounce_counter = -1; state.last_failed_time = timestamp; state.last_occurred = timestamp; - state.reporting_sources.insert(source_id); + add_source(state.reporting_sources); if (state.occurrence_count < std::numeric_limits::max()) { ++state.occurrence_count; } @@ -188,7 +199,7 @@ bool InMemoryFaultStorage::report_fault_event(const std::string & fault_code, ui state.debounce_counter = clamp_debounce_counter(state.debounce_counter - 1, config); // Add source if not already present - state.reporting_sources.insert(source_id); + add_source(state.reporting_sources); // Update severity if higher if (severity > state.severity) { diff --git a/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp b/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp index f6743afed..3037d5ebf 100644 --- a/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp @@ -44,6 +44,8 @@ class SqliteStatement { SqliteStatement(const SqliteStatement &) = delete; SqliteStatement & operator=(const SqliteStatement &) = delete; + SqliteStatement(SqliteStatement &&) = delete; + SqliteStatement & operator=(SqliteStatement &&) = delete; sqlite3_stmt * get() const { return stmt_; @@ -362,7 +364,8 @@ std::string SqliteFaultStorage::serialize_json_array(const std::vector lock(mutex_); int64_t timestamp_ns = timestamp.nanoseconds(); @@ -401,9 +404,12 @@ bool SqliteFaultStorage::report_fault_event(const std::string & fault_code, uint if (is_failed) { // FAILED event - // Parse existing sources and add new one + // Parse existing sources, drop a superseded one, then add the new source. std::vector sources = parse_json_array(sources_json); std::set sources_set(sources.begin(), sources.end()); + if (!supersedes_source_id.empty() && supersedes_source_id != source_id) { + sources_set.erase(supersedes_source_id); + } sources_set.insert(source_id); sources.assign(sources_set.begin(), sources_set.end()); diff --git a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp index e2c23bc15..83589abbf 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -109,6 +109,82 @@ TEST_F(FaultStorageTest, ReportExistingFaultEventUpdates) { EXPECT_EQ(fault->reporting_sources.size(), 2u); } +// Backward compatibility: with the supersedes_source_id argument left at its default +// (empty), a second source is simply added - the pre-existing multi-source behavior. +TEST_F(FaultStorageTest, ReportWithoutSupersedeKeepsBothSources) { + rclcpp::Clock clock; + storage_.report_fault_event("F", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "d", "/action_name", + clock.now(), default_config()); + storage_.report_fault_event("F", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "d", "/bt_navigator", + clock.now(), default_config()); // no supersede arg -> default "" + + auto fault = storage_.get_fault("F"); + ASSERT_TRUE(fault.has_value()); + const std::set sources(fault->reporting_sources.begin(), fault->reporting_sources.end()); + EXPECT_EQ(sources, (std::set{"/action_name", "/bt_navigator"})); +} + +// A supersede swaps the provisional source for the new one: the action-name source +// is dropped and only the resolved FQN remains (the #467 re-attribution flow). +TEST_F(FaultStorageTest, SupersedeReplacesProvisionalSource) { + rclcpp::Clock clock; + storage_.report_fault_event("ACTION_NAVIGATE_TO_POSE_ABORTED", ReportFault::Request::EVENT_FAILED, + Fault::SEVERITY_ERROR, "aborted", "/navigate_to_pose", clock.now(), default_config()); + bool is_new = storage_.report_fault_event("ACTION_NAVIGATE_TO_POSE_ABORTED", ReportFault::Request::EVENT_FAILED, + Fault::SEVERITY_ERROR, "aborted", "/bt_navigator", clock.now(), + default_config(), /*supersedes=*/"/navigate_to_pose"); + + EXPECT_FALSE(is_new); + auto fault = storage_.get_fault("ACTION_NAVIGATE_TO_POSE_ABORTED"); + ASSERT_TRUE(fault.has_value()); + const std::set sources(fault->reporting_sources.begin(), fault->reporting_sources.end()); + EXPECT_EQ(sources, (std::set{"/bt_navigator"})); // provisional source gone +} + +// Superseding an absent source is a no-op erase: the new source is still added. +TEST_F(FaultStorageTest, SupersedeAbsentSourceJustAddsNew) { + rclcpp::Clock clock; + storage_.report_fault_event("F", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "d", "/a", clock.now(), + default_config()); + storage_.report_fault_event("F", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "d", "/b", clock.now(), + default_config(), /*supersedes=*/"/never_reported"); + + auto fault = storage_.get_fault("F"); + ASSERT_TRUE(fault.has_value()); + const std::set sources(fault->reporting_sources.begin(), fault->reporting_sources.end()); + EXPECT_EQ(sources, (std::set{"/a", "/b"})); +} + +// Self-supersede guard: dropping the same source that is being added would leave the +// fault with no source at all. The store must treat supersedes == source_id as a no-op. +TEST_F(FaultStorageTest, SelfSupersedeKeepsTheSource) { + rclcpp::Clock clock; + storage_.report_fault_event("F", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "d", "/bt_navigator", + clock.now(), default_config()); + storage_.report_fault_event("F", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "d", "/bt_navigator", + clock.now(), default_config(), /*supersedes=*/"/bt_navigator"); + + auto fault = storage_.get_fault("F"); + ASSERT_TRUE(fault.has_value()); + const std::set sources(fault->reporting_sources.begin(), fault->reporting_sources.end()); + EXPECT_EQ(sources, (std::set{"/bt_navigator"})); +} + +// A supersede set on the very first report of a brand-new fault creates the fault and +// simply adds the source: the absent provisional source erase is a harmless no-op. +TEST_F(FaultStorageTest, SupersedeOnNewFaultJustAddsSource) { + rclcpp::Clock clock; + bool is_new = storage_.report_fault_event("F", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "d", + "/bt_navigator", clock.now(), default_config(), + /*supersedes=*/"/navigate_to_pose"); + + EXPECT_TRUE(is_new); + auto fault = storage_.get_fault("F"); + ASSERT_TRUE(fault.has_value()); + const std::set sources(fault->reporting_sources.begin(), fault->reporting_sources.end()); + EXPECT_EQ(sources, (std::set{"/bt_navigator"})); +} + TEST_F(FaultStorageTest, ListFaultsDefaultReturnsConfirmedOnly) { rclcpp::Clock clock; auto timestamp = clock.now(); diff --git a/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp b/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp index 586c3c7d1..3f8856911 100644 --- a/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp +++ b/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp @@ -298,6 +298,57 @@ TEST_F(SqliteFaultStorageTest, ReportingSourcesJsonHandling) { EXPECT_TRUE(sources.count("/special\"chars") > 0); } +// Supersede persists through the sqlite reporting_sources JSON column: the +// provisional source is removed from the stored set and only the FQN remains. +TEST_F(SqliteFaultStorageTest, SupersedeReplacesProvisionalSource) { + rclcpp::Clock clock; + auto ts = clock.now(); + storage_->report_fault_event("ACTION_ABORTED", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "aborted", + "/navigate_to_pose", ts, default_config()); + storage_->report_fault_event("ACTION_ABORTED", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "aborted", + "/bt_navigator", ts, default_config(), /*supersedes=*/"/navigate_to_pose"); + + auto fault = storage_->get_fault("ACTION_ABORTED"); + ASSERT_TRUE(fault.has_value()); + const std::set sources(fault->reporting_sources.begin(), fault->reporting_sources.end()); + EXPECT_EQ(sources, (std::set{"/bt_navigator"})); +} + +// Backward compatibility: default (empty) supersede keeps the existing add-source behavior. +TEST_F(SqliteFaultStorageTest, ReportWithoutSupersedeKeepsBothSources) { + rclcpp::Clock clock; + auto ts = clock.now(); + storage_->report_fault_event("F", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "d", "/a", ts, + default_config()); + storage_->report_fault_event("F", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "d", "/b", ts, + default_config()); // no supersede + + auto fault = storage_->get_fault("F"); + ASSERT_TRUE(fault.has_value()); + const std::set sources(fault->reporting_sources.begin(), fault->reporting_sources.end()); + EXPECT_EQ(sources, (std::set{"/a", "/b"})); +} + +// A supersede requested on the FAILED report that reactivates a CLEARED fault still +// drops the provisional source: reactivation reuses the FAILED source-merge branch. +TEST_F(SqliteFaultStorageTest, SupersedeAppliesAcrossReactivation) { + rclcpp::Clock clock; + auto ts = clock.now(); + storage_->report_fault_event("ACTION_ABORTED", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "aborted", + "/navigate_to_pose", ts, default_config()); + ASSERT_TRUE(storage_->clear_fault("ACTION_ABORTED")); + ASSERT_EQ(storage_->get_fault("ACTION_ABORTED")->status, Fault::STATUS_CLEARED); + + // Reactivating FAILED report, superseding the provisional source with the FQN. + storage_->report_fault_event("ACTION_ABORTED", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "aborted", + "/bt_navigator", clock.now(), default_config(), /*supersedes=*/"/navigate_to_pose"); + + auto fault = storage_->get_fault("ACTION_ABORTED"); + ASSERT_TRUE(fault.has_value()); + const std::set sources(fault->reporting_sources.begin(), fault->reporting_sources.end()); + EXPECT_EQ(sources, (std::set{"/bt_navigator"})); // provisional source gone +} + // Test database path accessor TEST_F(SqliteFaultStorageTest, DbPathAccessor) { EXPECT_EQ(storage_->db_path(), temp_db_path_.string()); diff --git a/src/ros2_medkit_fault_reporter/CHANGELOG.rst b/src/ros2_medkit_fault_reporter/CHANGELOG.rst index 51eb06477..da8f51095 100644 --- a/src/ros2_medkit_fault_reporter/CHANGELOG.rst +++ b/src/ros2_medkit_fault_reporter/CHANGELOG.rst @@ -2,6 +2,11 @@ Changelog for package ros2_medkit_fault_reporter ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Forthcoming +----------- +* ``FaultReporter::report`` gains an optional ``supersedes_source_id`` argument, forwarded to the ``ReportFault`` service so a caller can drop a previously-reported source when re-reporting a fault under a corrected source. When a supersede is requested the local debounce filter is bypassed so the correcting report is never dropped as a duplicate. Empty (default) preserves the prior behavior (`#467 `_) +* Contributors: @bburda + 0.6.0 (2026-06-22) ------------------ * No functional changes; version bump for the coordinated 0.6.0 release. diff --git a/src/ros2_medkit_fault_reporter/README.md b/src/ros2_medkit_fault_reporter/README.md index 69a08b74a..bd8388a2c 100644 --- a/src/ros2_medkit_fault_reporter/README.md +++ b/src/ros2_medkit_fault_reporter/README.md @@ -61,6 +61,22 @@ Report that a fault condition has cleared. Use this when FaultManager is configu reporter_->report_passed("MOTOR_OVERHEAT"); ``` +### `report(fault_code, severity, description, supersedes_source_id)` (Advanced) + +Pass a fourth argument to drop a previously-reported source from the fault's +`reporting_sources` before this reporter's source is added. Use it to correct a +provisional attribution - for example an action bridge that first reports a fault +under an action interface name, then re-reports under the resolved server node FQN +once discovery settles. A supersede bypasses local filtering so the correcting +report always reaches the FaultManager. + +```cpp +reporter_->report("MOTOR_OVERHEAT", + ros2_medkit_msgs::msg::Fault::SEVERITY_ERROR, + "Motor temperature exceeded safe limit", + "/navigate_to_pose"); // provisional source to drop +``` + ## Local Filtering The reporter includes optional local filtering to reduce noise from repeated fault occurrences. diff --git a/src/ros2_medkit_fault_reporter/include/ros2_medkit_fault_reporter/fault_reporter.hpp b/src/ros2_medkit_fault_reporter/include/ros2_medkit_fault_reporter/fault_reporter.hpp index 9e1696696..ee23eff04 100644 --- a/src/ros2_medkit_fault_reporter/include/ros2_medkit_fault_reporter/fault_reporter.hpp +++ b/src/ros2_medkit_fault_reporter/include/ros2_medkit_fault_reporter/fault_reporter.hpp @@ -63,7 +63,12 @@ class FaultReporter { /// @param fault_code Global fault identifier (e.g., "MOTOR_OVERHEAT") /// @param severity Severity level (use Fault::SEVERITY_* constants) /// @param description Human-readable fault description - void report(const std::string & fault_code, uint8_t severity, const std::string & description); + /// @param supersedes_source_id Optional. A previously-reported source to drop from the fault's + /// reporting_sources before this reporter's source is added, so a provisional attribution + /// can be corrected (empty = no supersede). Bypasses local filtering when set, since a + /// re-attribution must always reach the FaultManager. + void report(const std::string & fault_code, uint8_t severity, const std::string & description, + const std::string & supersedes_source_id = ""); /// Report a PASSED event (fault condition cleared) /// @@ -87,7 +92,7 @@ class FaultReporter { /// Send the fault report to FaultManager (async, fire-and-forget) void send_report(const std::string & fault_code, uint8_t event_type, uint8_t severity, - const std::string & description); + const std::string & description, const std::string & supersedes_source_id = ""); std::string source_id_; rclcpp::Client::SharedPtr client_; diff --git a/src/ros2_medkit_fault_reporter/src/fault_reporter.cpp b/src/ros2_medkit_fault_reporter/src/fault_reporter.cpp index b08725b6e..28d82f6dc 100644 --- a/src/ros2_medkit_fault_reporter/src/fault_reporter.cpp +++ b/src/ros2_medkit_fault_reporter/src/fault_reporter.cpp @@ -63,20 +63,24 @@ void FaultReporter::load_parameters(const rclcpp::Node::SharedPtr & node) { config.default_threshold, config.default_window_sec, config.bypass_severity); } -void FaultReporter::report(const std::string & fault_code, uint8_t severity, const std::string & description) { +void FaultReporter::report(const std::string & fault_code, uint8_t severity, const std::string & description, + const std::string & supersedes_source_id) { // Validate fault_code if (fault_code.empty()) { RCLCPP_WARN(logger_, "Attempted to report fault with empty fault_code, ignoring"); return; } - // Check if filter allows forwarding - if (!filter_.should_forward(fault_code, severity)) { + // A supersede is a re-attribution of an already-reported fault, not a fresh + // occurrence, so it must always reach the FaultManager - never drop it to the + // local filter (which would leave the provisional source in place). + if (supersedes_source_id.empty() && !filter_.should_forward(fault_code, severity)) { RCLCPP_DEBUG(logger_, "Fault '%s' filtered (threshold not met)", fault_code.c_str()); return; } - send_report(fault_code, ros2_medkit_msgs::srv::ReportFault::Request::EVENT_FAILED, severity, description); + send_report(fault_code, ros2_medkit_msgs::srv::ReportFault::Request::EVENT_FAILED, severity, description, + supersedes_source_id); } void FaultReporter::report_passed(const std::string & fault_code) { @@ -98,7 +102,7 @@ bool FaultReporter::is_service_ready() const { } void FaultReporter::send_report(const std::string & fault_code, uint8_t event_type, uint8_t severity, - const std::string & description) { + const std::string & description, const std::string & supersedes_source_id) { if (!client_->service_is_ready()) { // Use WARN level for high-severity faults that would bypass filtering if (event_type == ros2_medkit_msgs::srv::ReportFault::Request::EVENT_FAILED && @@ -116,6 +120,7 @@ void FaultReporter::send_report(const std::string & fault_code, uint8_t event_ty request->severity = severity; request->description = description; request->source_id = source_id_; + request->supersedes_source_id = supersedes_source_id; // Fire and forget - don't block on response client_->async_send_request(request); diff --git a/src/ros2_medkit_integration_tests/test/features/test_fault_source_supersede.test.py b/src/ros2_medkit_integration_tests/test/features/test_fault_source_supersede.test.py new file mode 100644 index 000000000..62444e141 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_fault_source_supersede.test.py @@ -0,0 +1,174 @@ +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""End-to-end regression test for provisional-source re-attribution (#467). + +A reporter that does not yet know the entity's node FQN (e.g. the +action_status_bridge before DDS discovery resolves the server node) reports a +fault under a provisional source - an action interface name that maps to no +SOVD entity. Under the strict-AND per-entity /faults scope filter that fault is +correctly kept out of every node entity. + +ReportFault.supersedes_source_id lets the reporter correct the attribution: it +re-reports under the resolved FQN and names the provisional source to drop, so +the FaultManager ends with reporting_sources = {FQN} and the fault resolves to +the server node's entity. This drives that full path through the real gateway +scope filter: provisional -> 404 under the entity, then supersede -> 200. + +@verifies REQ_INTEROP_013 +""" + +import os +import unittest + +from ament_index_python.packages import get_package_share_directory +import launch_testing +import rclpy +from rclpy.node import Node +import requests + +from ros2_medkit_msgs.msg import Fault +from ros2_medkit_msgs.srv import ReportFault +from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES +from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase +from ros2_medkit_test_utils.launch_helpers import create_test_launch + + +# The manifest maps this node FQN to the 'lidar-sensor' app on the 'lidar-unit' +# component (same mapping the scope-isolation regression test relies on). +SERVER_FQN = '/perception/lidar/lidar_sensor' +OWNER_APP = 'lidar-sensor' +# A provisional source that maps to no SOVD entity - stands in for the action +# interface name the action_status_bridge falls back to before discovery. +PROVISIONAL_SOURCE = '/navigate_to_pose' +FAULT_CODE = 'ACTION_NAVIGATE_TO_POSE_ABORTED' + + +def generate_test_description(): + manifest_path = os.path.join( + get_package_share_directory('ros2_medkit_gateway'), + 'config', 'examples', 'demo_nodes_manifest.yaml', + ) + return create_test_launch( + demo_nodes=['lidar_sensor'], + fault_manager=True, + # Negative threshold: a fault confirms after a couple of FAILED events. + fault_manager_params={'confirmation_threshold': -2}, + gateway_params={ + 'discovery.mode': 'hybrid', + 'discovery.manifest_path': manifest_path, + 'discovery.manifest_strict_validation': False, + 'discovery.merge_pipeline.gap_fill.allow_heuristic_apps': True, + }, + ) + + +class TestFaultSourceSupersede(GatewayTestCase): + """A provisional source is corrected to the server FQN via supersede.""" + + MIN_EXPECTED_APPS = 1 + REQUIRED_APPS = {OWNER_APP} + + @classmethod + def setUpClass(cls): + super().setUpClass() + rclpy.init() + cls._reporter = Node('supersede_fault_reporter') + cls._report_client = cls._reporter.create_client( + ReportFault, '/fault_manager/report_fault' + ) + assert cls._report_client.wait_for_service(timeout_sec=15.0), \ + 'report_fault service not available' + + @classmethod + def tearDownClass(cls): + cls._reporter.destroy_node() + rclpy.shutdown() + + def _report(self, source_id, supersedes='', times=4): + """Fire-and-forget FAILED reports. + + Drop the reply future to avoid sanitizer-timing flakes; report several + times so the negative confirmation_threshold latches CONFIRMED. + """ + for _ in range(times): + req = ReportFault.Request() + req.fault_code = FAULT_CODE + req.event_type = ReportFault.Request.EVENT_FAILED + req.severity = Fault.SEVERITY_ERROR + req.description = 'Action /navigate_to_pose aborted' + req.source_id = source_id + req.supersedes_source_id = supersedes + self._report_client.call_async(req) + rclpy.spin_once(self._reporter, timeout_sec=0.1) + + def _fault_status_on_owner(self): + return requests.get( + f'{self.BASE_URL}/apps/{OWNER_APP}/faults/{FAULT_CODE}', timeout=10, + ).status_code + + def test_supersede_moves_fault_to_server_entity(self): + """Provisional attribution is invisible to the entity; supersede fixes it. + + @verifies REQ_INTEROP_013 + """ + # 1. Report under the provisional (action-name) source. It maps to no + # entity, so the strict-AND scope filter keeps it off the owning app. + self._report(PROVISIONAL_SOURCE) + self._poll_until( + lambda: self._fault_status_on_owner() == 404, + 'fault reported under a provisional source must not surface under ' + 'the server entity', + ) + + # 2. Re-report under the resolved FQN, superseding the provisional + # source. The FaultManager drops the provisional source, leaving + # reporting_sources = {FQN}, so the fault now resolves to the entity. + self._report(SERVER_FQN, supersedes=PROVISIONAL_SOURCE) + self._poll_until( + lambda: self._fault_status_on_owner() == 200, + 'after supersede the fault must resolve to the server entity', + ) + + # The corrected fault detail carries the FQN and not the provisional source. + detail = self.get_json(f'/apps/{OWNER_APP}/faults/{FAULT_CODE}') + sources = detail.get('item', {}).get('reporting_sources', []) + self.assertIn(SERVER_FQN, sources) + self.assertNotIn(PROVISIONAL_SOURCE, sources) + + def _poll_until(self, predicate, message, timeout=20.0, interval=0.5): + import time + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + try: + if predicate(): + return + except requests.RequestException: + pass + rclpy.spin_once(self._reporter, timeout_sec=0.05) + time.sleep(interval) + self.fail(f'Timed out after {timeout}s: {message}') + + +@launch_testing.post_shutdown_test() +class TestShutdown(unittest.TestCase): + """Confirm the gateway and fault_manager exited cleanly.""" + + def test_exit_codes(self, proc_info): + """Check all processes exited cleanly (SIGTERM allowed).""" + for info in proc_info: + self.assertIn( + info.returncode, ALLOWED_EXIT_CODES, + f'{info.process_name} exited with code {info.returncode}' + ) diff --git a/src/ros2_medkit_msgs/CHANGELOG.rst b/src/ros2_medkit_msgs/CHANGELOG.rst index 9b74117b6..7c3bc3387 100644 --- a/src/ros2_medkit_msgs/CHANGELOG.rst +++ b/src/ros2_medkit_msgs/CHANGELOG.rst @@ -2,6 +2,11 @@ Changelog for package ros2_medkit_msgs ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Forthcoming +----------- +* ``ReportFault.srv`` request gains an optional ``string supersedes_source_id`` field: a reporter can drop a previously-reported source from the fault's ``reporting_sources`` when re-reporting under a corrected source, so a fault first attributed to a provisional source (an action bridge before DDS discovery resolves the server node FQN) resolves to the right entity under the strict-AND per-entity ``/faults`` scope filter. Empty (default) preserves the prior add-only behavior; out-of-tree callers must rebuild against the new message (`#467 `_) +* Contributors: @bburda + 0.6.0 (2026-06-22) ------------------ * No functional changes; version bump for the coordinated 0.6.0 release. diff --git a/src/ros2_medkit_msgs/README.md b/src/ros2_medkit_msgs/README.md index 597747a34..f5c12334b 100644 --- a/src/ros2_medkit_msgs/README.md +++ b/src/ros2_medkit_msgs/README.md @@ -85,6 +85,7 @@ Report a fault event (FAILED or PASSED) to the FaultManager. | `severity` | uint8 | Severity level (0-3, only for FAILED events) | | `description` | string | Human-readable description (only for FAILED events) | | `source_id` | string | Reporting node FQN (e.g., "/powertrain/engine/temp_sensor") | +| `supersedes_source_id` | string | Optional. A previously-reported source to drop from `reporting_sources` before `source_id` is added (empty = no supersede). Corrects a provisional attribution so the fault resolves to the right entity under the strict-AND per-entity scope filter. | **Response:** | Field | Type | Description | diff --git a/src/ros2_medkit_msgs/srv/ReportFault.srv b/src/ros2_medkit_msgs/srv/ReportFault.srv index c6403ca9f..50075e1ad 100644 --- a/src/ros2_medkit_msgs/srv/ReportFault.srv +++ b/src/ros2_medkit_msgs/srv/ReportFault.srv @@ -49,6 +49,14 @@ string description # including namespace (e.g., "/powertrain/engine/temp_sensor"). # This value is added to the fault's reporting_sources array for multi-source tracking. string source_id + +# Optional. A previously-reported source_id to drop from this fault's reporting_sources +# before source_id is added. Empty = no supersede (default). Lets a reporter correct a +# provisional attribution - e.g. an action bridge that first reports a fault under the +# action interface name, then re-reports it under the resolved server node FQN once DDS +# discovery settles - so the fault resolves to the right entity under the strict-AND +# per-entity /faults scope filter. Only meaningful for FAILED events on an existing fault. +string supersedes_source_id --- # Response fields From ecb988015c9cb5fdd30b286b7a1ce0946c7c19c7 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 16 Jul 2026 11:39:28 +0200 Subject: [PATCH 2/3] Add deterministic end-to-end regression for #467 re-attribution 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. --- .../CMakeLists.txt | 18 ++ .../test/slow_discovery_bridge_main.cpp | 102 ++++++++ .../test/test_reattribution_e2e.test.py | 217 ++++++++++++++++++ 3 files changed, 337 insertions(+) create mode 100644 src/ros2_medkit_action_status_bridge/test/slow_discovery_bridge_main.cpp create mode 100644 src/ros2_medkit_action_status_bridge/test/test_reattribution_e2e.test.py diff --git a/src/ros2_medkit_action_status_bridge/CMakeLists.txt b/src/ros2_medkit_action_status_bridge/CMakeLists.txt index db0713b3f..aadebf26a 100644 --- a/src/ros2_medkit_action_status_bridge/CMakeLists.txt +++ b/src/ros2_medkit_action_status_bridge/CMakeLists.txt @@ -117,6 +117,24 @@ if(BUILD_TESTING) TIMEOUT 120 ) + # Deterministic end-to-end regression for #467 provisional re-attribution. + # slow_discovery_bridge is the real ActionStatusBridgeNode with only the racy + # DDS-FQN resolution forced to "" for the first few rescans, so the real + # provisional -> supersede path runs against a real FaultManager. + add_executable(slow_discovery_bridge test/slow_discovery_bridge_main.cpp) + target_link_libraries(slow_discovery_bridge action_status_bridge_lib) + medkit_target_dependencies(slow_discovery_bridge rclcpp) + install(TARGETS slow_discovery_bridge DESTINATION lib/${PROJECT_NAME}) + + add_launch_test( + test/test_reattribution_e2e.test.py + TARGET test_reattribution_e2e + TIMEOUT 120 + ) + set_tests_properties(test_reattribution_e2e PROPERTIES + ENVIRONMENT "ROS_DOMAIN_ID=${_MEDKIT_DOMAIN_COUNTER}") + math(EXPR _MEDKIT_DOMAIN_COUNTER "${_MEDKIT_DOMAIN_COUNTER} + 1") + ros2_medkit_relax_vendor_warnings() endif() diff --git a/src/ros2_medkit_action_status_bridge/test/slow_discovery_bridge_main.cpp b/src/ros2_medkit_action_status_bridge/test/slow_discovery_bridge_main.cpp new file mode 100644 index 000000000..52fe32a51 --- /dev/null +++ b/src/ros2_medkit_action_status_bridge/test/slow_discovery_bridge_main.cpp @@ -0,0 +1,102 @@ +// Copyright 2026 bburda +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Test-only executable: the real ActionStatusBridgeNode with a deterministic +// stand-in for the one inherently racy primitive - resolving the action server's +// node FQN from DDS discovery. Everything else (reporter_for, the provisional +// bookkeeping, reattribute_provisional, report()+supersede, the timers) is the +// unmodified production code, exercised end to end against a real FaultManager. +// +// This reproduces issue #467's "abort before DDS discovery resolves the server +// FQN" race without depending on real DDS timing: the FIRST few resolutions for +// an action return "" (as rcl does while a participant's node name has not yet +// propagated), so the bridge falls back to the provisional action-name source +// and reports the fault under it. A later resolution returns the real FQN, so +// the rescan's reattribute_provisional() re-reports under the FQN with +// supersedes_source_id set and the FaultManager drops the provisional source. + +#include +#include +#include + +#include "rclcpp/rclcpp.hpp" +#include "ros2_medkit_action_status_bridge/action_status_bridge_node.hpp" + +namespace { + +// server_fqn_for_action is a private virtual (a Non-Virtual-Interface +// customization point); a derived class may still override it, and the base's +// internal calls dispatch here through the vtable. +class SlowDiscoveryBridge : public ros2_medkit_action_status_bridge::ActionStatusBridgeNode { + public: + SlowDiscoveryBridge() : ActionStatusBridgeNode() { + } + ~SlowDiscoveryBridge() override = default; + SlowDiscoveryBridge(const SlowDiscoveryBridge &) = delete; + SlowDiscoveryBridge & operator=(const SlowDiscoveryBridge &) = delete; + SlowDiscoveryBridge(SlowDiscoveryBridge &&) = delete; + SlowDiscoveryBridge & operator=(SlowDiscoveryBridge &&) = delete; + + private: + // Number of leading resolutions per action that are forced to "" (unresolved). + // Call 1 is reporter_for's first report (-> provisional); the next few are + // reattribute_provisional rescans that keep the provisional entry; after that + // the real FQN is returned and the supersede fires. Keeping it > 1 makes the + // provisional state observable across several rescans instead of a single tick. + static constexpr int kForcedUnresolvedCalls = 3; + + std::string server_fqn_for_action(const std::string & action_name) override { + { + std::lock_guard lock(mutex_); + int & n = calls_[action_name]; + if (n < kForcedUnresolvedCalls) { + ++n; + return ""; // simulate slow discovery: force the provisional fallback + } + } + // Resolve for real via the public graph API, mirroring the production + // server_fqn_for_action / server_fqn_from_endpoint logic. + constexpr const char * kUnknownName = "_NODE_NAME_UNKNOWN_"; + constexpr const char * kUnknownNamespace = "_NODE_NAMESPACE_UNKNOWN_"; + for (const auto & p : get_publishers_info_by_topic(action_name + "/_action/status")) { + const std::string & name = p.node_name(); + const std::string & ns = p.node_namespace(); + if (name.empty() || name == kUnknownName || ns == kUnknownNamespace) { + continue; + } + std::string fqn; + if (ns.empty() || ns == "/") { + fqn = "/"; + } else { + fqn = ns; + fqn += "/"; + } + fqn += name; + return fqn; + } + return ""; + } + + std::mutex mutex_; + std::map calls_; +}; + +} // namespace + +int main(int argc, char * argv[]) { + rclcpp::init(argc, argv); + rclcpp::spin(std::make_shared()); + rclcpp::shutdown(); + return 0; +} diff --git a/src/ros2_medkit_action_status_bridge/test/test_reattribution_e2e.test.py b/src/ros2_medkit_action_status_bridge/test/test_reattribution_e2e.test.py new file mode 100644 index 000000000..c47fcac50 --- /dev/null +++ b/src/ros2_medkit_action_status_bridge/test/test_reattribution_e2e.test.py @@ -0,0 +1,217 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""End-to-end regression for provisional-source re-attribution (issue #467). + +This drives the REAL action_status_bridge (not a hand-rolled reporter) against a +real FaultManager. The only substitution is a test-double executable that forces +the inherently racy DDS-FQN resolution to return "" for the first few rescans, +reproducing the "action aborts before discovery resolves the server FQN" race +deterministically. Everything else - reporter_for's provisional bookkeeping, +reattribute_provisional(), report() with supersedes_source_id, the rescan timer - +is the unmodified production bridge. + +Asserted end to end: + 1. the fault first surfaces under the provisional action-name source + (/test_action), because the FQN was unresolved at first report; then + 2. once the FQN resolves, the bridge re-reports under it with the provisional + source superseded, so reporting_sources ends as {FQN} with the provisional + source dropped. + +Step 2 is the exact behavior that was impossible before #467 (append-only +reporting_sources + strict-AND per-entity scope filter left the fault stuck +under the provisional source forever). +""" + +import time +import unittest + +from action_msgs.msg import GoalStatus, GoalStatusArray +from launch import LaunchDescription +import launch_ros.actions +import launch_testing.actions +import launch_testing.markers +import rclpy +from rclpy.node import Node +from rclpy.qos import ( + DurabilityPolicy, + HistoryPolicy, + QoSProfile, + ReliabilityPolicy, +) +from ros2_medkit_msgs.srv import ListFaults +from unique_identifier_msgs.msg import UUID + +STATUS_TOPIC = '/test_action/_action/status' +ABORTED_CODE = 'ACTION_TEST_ACTION_ABORTED' +# The action-interface name the bridge falls back to before discovery resolves. +PROVISIONAL_SOURCE = '/test_action' +# This test node publishes the status topic, so it IS the action server: its node +# FQN is what the fault must be re-attributed to. +SERVER_FQN = '/test_action_status_client' + + +def generate_test_description(): + """fault_manager + the slow-discovery test-double bridge.""" + fault_manager_node = launch_ros.actions.Node( + package='ros2_medkit_fault_manager', + executable='fault_manager_node', + name='fault_manager', + output='screen', + parameters=[{ + 'storage_type': 'memory', + 'confirmation_threshold': -1, # confirm on the first FAILED + 'healing_enabled': True, + 'healing_threshold': 0, + }], + ) + # Test-double: real bridge, deterministic slow-discovery FQN resolution. + slow_discovery_bridge = launch_ros.actions.Node( + package='ros2_medkit_action_status_bridge', + executable='slow_discovery_bridge', + name='action_status_bridge', + output='screen', + parameters=[{ + 'rescan_period_sec': 1.0, # fast rescans so re-attribution is prompt + }], + ) + return ( + LaunchDescription([ + fault_manager_node, + slow_discovery_bridge, + launch_testing.actions.ReadyToTest(), + ]), + { + 'fault_manager_node': fault_manager_node, + 'slow_discovery_bridge': slow_discovery_bridge, + }, + ) + + +class TestReattributionEndToEnd(unittest.TestCase): + """A fault reported under a provisional source is re-attributed to the FQN.""" + + @classmethod + def setUpClass(cls): + rclpy.init() + cls.node = Node('test_action_status_client') + # Match the bridge's subscription QoS so the latched status is delivered. + qos = QoSProfile( + depth=10, + reliability=ReliabilityPolicy.RELIABLE, + durability=DurabilityPolicy.TRANSIENT_LOCAL, + history=HistoryPolicy.KEEP_LAST, + ) + cls.status_pub = cls.node.create_publisher(GoalStatusArray, STATUS_TOPIC, qos) + cls.list_faults_client = cls.node.create_client( + ListFaults, '/fault_manager/list_faults' + ) + assert cls.list_faults_client.wait_for_service(timeout_sec=15.0), \ + 'ListFaults service not available' + + @classmethod + def tearDownClass(cls): + cls.node.destroy_node() + rclpy.shutdown() + + def _reporting_sources(self, code): + """reporting_sources for a fault_code, or None if the fault is absent.""" + request = ListFaults.Request() + request.filter_by_severity = False + request.statuses = [] + future = self.list_faults_client.call_async(request) + rclpy.spin_until_future_complete(self.node, future, timeout_sec=5.0) + result = future.result() + if result is None: + return None + for fault in result.faults: + if fault.fault_code == code: + return list(fault.reporting_sources) + return None + + def _wait_for_bridge_watching(self, timeout=15.0): + deadline = time.time() + timeout + while time.time() < deadline: + infos = self.node.get_subscriptions_info_by_topic(STATUS_TOPIC) + if any(info.node_name == 'action_status_bridge' for info in infos): + return True + rclpy.spin_once(self.node, timeout_sec=0.1) + return False + + def _spin(self, seconds): + end = time.time() + seconds + while time.time() < end: + rclpy.spin_once(self.node, timeout_sec=0.05) + + def test_provisional_fault_is_reattributed_to_server_fqn(self): + """Provisional (action-name) source -> superseded -> server FQN.""" + self.assertTrue( + self._wait_for_bridge_watching(), + 'bridge did not subscribe to the status topic in time', + ) + # Abort a goal. The test-double resolves the FQN as "" for the first few + # rescans, so this first report lands under the provisional source. + self.status_pub.publish(_status_array(GoalStatus.STATUS_ABORTED, 1)) + + # 1. The fault must appear under the provisional action-name source. Poll + # for a bounded window and record that we observed it there - this is + # the state that used to be permanent before #467. + saw_provisional = False + deadline = time.time() + 20.0 + while time.time() < deadline: + sources = self._reporting_sources(ABORTED_CODE) + if sources is not None and PROVISIONAL_SOURCE in sources and SERVER_FQN not in sources: + saw_provisional = True + break + self._spin(0.2) + self.assertTrue( + saw_provisional, + 'fault never surfaced under the provisional action-name source ' + f'{PROVISIONAL_SOURCE!r}', + ) + + # 2. Once discovery resolves, reattribute_provisional() re-reports under + # the FQN with the provisional source superseded. The fault must end + # with the FQN present and the provisional source dropped. + reattributed = False + deadline = time.time() + 30.0 + while time.time() < deadline: + sources = self._reporting_sources(ABORTED_CODE) + if sources is not None and SERVER_FQN in sources and PROVISIONAL_SOURCE not in sources: + reattributed = True + break + self._spin(0.2) + self.assertTrue( + reattributed, + 'fault was not re-attributed to the server FQN ' + f'{SERVER_FQN!r} (provisional source not superseded)', + ) + + +def _status_array(status, goal_byte): + gsa = GoalStatusArray() + gs = GoalStatus() + gs.goal_info.goal_id = UUID(uuid=[goal_byte] * 16) + gs.status = int(status) + gsa.status_list = [gs] + return gsa + + +@launch_testing.post_shutdown_test() +class TestReattributionShutdown(unittest.TestCase): + """Confirm the fault_manager and bridge exited cleanly.""" + + def test_exit_codes(self, proc_info): + launch_testing.asserts.assertExitCodes(proc_info) From d87024d6d0c5258f8442035214b0c9aa648fb8c9 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Fri, 17 Jul 2026 14:53:05 +0200 Subject: [PATCH 3/3] Fix supersede test assertion and action_status_bridge lint gates - 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). --- .../src/action_status_bridge_node.cpp | 10 ++++++++-- .../test/test_reattribution_e2e.test.py | 3 ++- .../test/features/test_fault_source_supersede.test.py | 7 +++++-- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/ros2_medkit_action_status_bridge/src/action_status_bridge_node.cpp b/src/ros2_medkit_action_status_bridge/src/action_status_bridge_node.cpp index 34cda0c20..f525d4309 100644 --- a/src/ros2_medkit_action_status_bridge/src/action_status_bridge_node.cpp +++ b/src/ros2_medkit_action_status_bridge/src/action_status_bridge_node.cpp @@ -496,8 +496,14 @@ ros2_medkit_fault_reporter::FaultReporter * ActionStatusBridgeNode::reporter_for } 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. + // Snapshot the provisional action names under reporters_mutex_, then release it + // so the per-action pass does not iterate provisional_ while it mutates the map + // (it erases corrected entries below). Each action's delivered state is read + // under state_mutex_; the graph query and re-report then run under reporters_mutex_ + // - held while we look up, cache the FQN reporter in, and erase from provisional_, + // the same lock reporter_for() takes, so the correction stays atomic against + // concurrent status callbacks. Neither server_fqn_for_action() nor + // FaultReporter::report() re-acquires reporters_mutex_, so there is no deadlock. std::vector actions; { std::lock_guard lock(reporters_mutex_); diff --git a/src/ros2_medkit_action_status_bridge/test/test_reattribution_e2e.test.py b/src/ros2_medkit_action_status_bridge/test/test_reattribution_e2e.test.py index c47fcac50..9820db695 100644 --- a/src/ros2_medkit_action_status_bridge/test/test_reattribution_e2e.test.py +++ b/src/ros2_medkit_action_status_bridge/test/test_reattribution_e2e.test.py @@ -13,7 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. -"""End-to-end regression for provisional-source re-attribution (issue #467). +""" +End-to-end regression for provisional-source re-attribution (issue #467). This drives the REAL action_status_bridge (not a hand-rolled reporter) against a real FaultManager. The only substitution is a test-double executable that forces diff --git a/src/ros2_medkit_integration_tests/test/features/test_fault_source_supersede.test.py b/src/ros2_medkit_integration_tests/test/features/test_fault_source_supersede.test.py index 62444e141..ab7bfd769 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_fault_source_supersede.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_fault_source_supersede.test.py @@ -1,3 +1,4 @@ +#!/usr/bin/env python3 # Copyright 2026 bburda # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -141,9 +142,11 @@ def test_supersede_moves_fault_to_server_entity(self): 'after supersede the fault must resolve to the server entity', ) - # The corrected fault detail carries the FQN and not the provisional source. + # The corrected fault detail carries the FQN and not the provisional + # source. reporting_sources lives under the x-medkit extension object, + # not the SOVD "item" block (which only carries code/name/severity/status). detail = self.get_json(f'/apps/{OWNER_APP}/faults/{FAULT_CODE}') - sources = detail.get('item', {}).get('reporting_sources', []) + sources = detail.get('x-medkit', {}).get('reporting_sources', []) self.assertIn(SERVER_FQN, sources) self.assertNotIn(PROVISIONAL_SOURCE, sources)