From 3f6a5bf45f7629bb60fdfd8779c15821a60cba00 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 20 Aug 2026 17:03:05 +0200 Subject: [PATCH 1/9] feat(fault_manager): retain a bounded near-miss series per fault code A FAILED report that moved the debounce counter without confirming the fault left no trace beyond the counter itself, and the counter is updated in place. Each near miss therefore overwrote the last, so nothing recorded how often a fault code approached confirmation. Append one row per such report to a new near_misses table (the in-memory backend keeps the equivalent per-code series), holding the timestamp, the counter value after the report, the confirmation threshold it was measured against, the severity and the reporting source. PASSED reports move the counter in the healing direction and are not near misses. The series is left alone by clear_fault and by the startup reclassification of HEALED faults: acknowledging one fault cycle must not erase a record that spans cycles and cannot be reconstructed afterwards. Per-topic snapshots keep their existing clear-on-acknowledge behaviour, since they belong to the single confirmed occurrence. Retention is bounded per fault code by the new near_miss.max_per_fault parameter (default 200, 0 = unlimited), evicting the oldest entries first - deliberately the opposite of the snapshot limit's keep-earliest rule, because a series frozen at boot says nothing about whether the rate is changing. A fault database written by an earlier build gains the table on first open. --- docs/config/fault-manager.rst | 39 +++ src/ros2_medkit_fault_manager/README.md | 29 ++ .../config/fault_manager.yaml | 6 + .../fault_storage.hpp | 56 +++- .../sqlite_fault_storage.hpp | 18 +- .../src/fault_manager_node.cpp | 16 + .../src/fault_storage.cpp | 50 +++ .../src/sqlite_fault_storage.cpp | 102 ++++++ .../test/test_fault_manager.cpp | 154 +++++++++ .../test/test_sqlite_storage.cpp | 296 ++++++++++++++++++ 10 files changed, 764 insertions(+), 2 deletions(-) diff --git a/docs/config/fault-manager.rst b/docs/config/fault-manager.rst index 45c6fb2eb..b548e7a0f 100644 --- a/docs/config/fault-manager.rst +++ b/docs/config/fault-manager.rst @@ -75,6 +75,45 @@ The fault manager uses AUTOSAR DEM-style debounce filtering to prevent fault fla For immediate fault confirmation (no debounce), set ``confirmation_threshold: 0``. Faults with ``SEVERITY_CRITICAL`` always bypass debounce regardless of this setting. +Near-Miss Retention +~~~~~~~~~~~~~~~~~~~ + +A **near miss** is a FAILED report that moved the debounce counter without the fault ending up +CONFIRMED - the fault nearly happened. PASSED reports move the counter in the healing direction +(the fault receding) and are not near misses. + +The fault manager appends one entry per near miss to a per-fault-code series, holding the +timestamp, the counter value after the report, the confirmation threshold, the severity and the +reporting source. The series is **retained when the fault is cleared**, because acknowledging one +fault cycle must not erase how often that code approached confirmation across cycles. + +.. code-block:: yaml + + fault_manager: + ros__parameters: + near_miss: + max_per_fault: 200 # Entries kept per fault code (0 = unlimited) + +.. list-table:: + :header-rows: 1 + :widths: 30 12 58 + + * - Parameter + - Default + - Description + * - ``near_miss.max_per_fault`` + - ``200`` + - Near-miss entries retained per fault code. When the bound is reached the **oldest** + entries are evicted - the opposite of ``snapshots.max_per_fault``, which keeps the + earliest, because a series frozen at boot says nothing about whether the rate of near + misses is changing. Set to 0 for unlimited, accepting growth with the reporting rate. + +.. note:: + + The series lives in the ``near_misses`` table of the fault database and is read through the + storage API (``FaultStorage::get_near_misses``). A database written by an earlier build gains + the table on first open. There is no service or REST surface for it yet. + Per-Entity Thresholds ~~~~~~~~~~~~~~~~~~~~~ diff --git a/src/ros2_medkit_fault_manager/README.md b/src/ros2_medkit_fault_manager/README.md index a16ab9d34..914b3b0e2 100644 --- a/src/ros2_medkit_fault_manager/README.md +++ b/src/ros2_medkit_fault_manager/README.md @@ -52,6 +52,7 @@ ros2 service call /fault_manager/clear_fault ros2_medkit_msgs/srv/ClearFault \ - **Persistent storage**: SQLite backend ensures faults survive node restarts - **Debounce filtering** (optional): AUTOSAR DEM-style counter-based fault confirmation with per-entity threshold overrides - **Snapshot capture**: Captures topic data when faults are confirmed for debugging (snapshots are deleted when fault is cleared) +- **Near-miss series**: Appends one entry per FAILED report that moved the debounce counter without confirming, bounded per fault code and retained when the fault is cleared - **Freeze-frame retention**: One compact JSON freeze-frame per fault code, retained across `clear_fault` (see below) - **Fault correlation** (optional): Root cause analysis with symptom muting and auto-clear - **Tamper-evident audit log** (optional): Append-only, hash-chained record of fault state transitions for verifiable history @@ -67,6 +68,7 @@ ros2 service call /fault_manager/clear_fault ros2_medkit_msgs/srv/ClearFault \ | `healing_threshold` | int | `3` | Counter value at which faults are healed | | `auto_confirm_after_sec` | double | `0.0` | Auto-confirm PREFAILED faults after timeout (0 = disabled) | | `entity_thresholds.config_file` | string | `""` | Path to YAML file with per-entity debounce threshold overrides | +| `near_miss.max_per_fault` | int | `200` | Near-miss entries retained per fault code, oldest evicted first (0 = unlimited) | ### Snapshot Parameters @@ -132,6 +134,33 @@ format used by black-box capture (`snapshots.rosbag.format`, see Rosbag Capture **Memory**: Faults are stored in memory only. Useful for testing or when persistence is not required. +## Near-Miss Series + +A **near miss** is a FAILED report that moved the debounce counter without the fault ending up +CONFIRMED - the fault nearly happened. This is the debounce sense of the term and is unrelated +to any scoring sense used elsewhere. PASSED reports move the counter too, but in the healing +direction (the fault receding), so they are not near misses. + +The series is **append-only**: one entry per qualifying report, holding the timestamp, the counter +value after the report, the confirmation threshold it was measured against, the severity and the +reporting source. Nothing is updated in place, so "this code approached confirmation five times +this hour" stays answerable. + +It is **retained across `~/clear_fault`**. Clearing acknowledges one fault cycle; how often a code +approaches confirmation spans cycles, and once deleted it cannot be reconstructed. The same holds +for the startup reclassification of HEALED faults. Per-topic snapshots are still dropped on clear - +they belong to the one confirmed occurrence, not to the series. + +Retention is **bounded per fault code** by `near_miss.max_per_fault` (default 200), evicting the +**oldest** entries first. That is deliberately the opposite of the snapshot limit's keep-earliest +rule: a series frozen at boot says nothing about whether the rate is changing. Set it to 0 for +unlimited, accepting that the database then grows with the reporting rate. + +The series lives in the `near_misses` table of the fault database and is read through the storage +API (`FaultStorage::get_near_misses`). A database written by an earlier build gains the table on +first open. **There is no service or REST surface for it yet** - reading it means going through the +storage API or the database file. + ## Advanced: Tamper-Evident Audit Log An optional append-only, hash-chained audit log records every fault state transition (`occurred`, `confirmed`, `healed`, `cleared`) so the fault history is independently verifiable. Auto-recovery (a fault reaching the healing threshold via PASSED events) is recorded as a distinct `healed` row with source `auto_heal`, so the fault's END is in the timeline and is not confused with a manual `cleared`. The manager has no acknowledge action separate from clearing, so `~/clear_fault` is recorded as `cleared` (clear == ack); there is no `ack` kind. The log also records its own lifecycle with `logging_activated` / `logging_deactivated` markers at start and stop. It is **off by default** because it adds a write and storage cost per transition. diff --git a/src/ros2_medkit_fault_manager/config/fault_manager.yaml b/src/ros2_medkit_fault_manager/config/fault_manager.yaml index 27e7a5af5..4dd549a67 100644 --- a/src/ros2_medkit_fault_manager/config/fault_manager.yaml +++ b/src/ros2_medkit_fault_manager/config/fault_manager.yaml @@ -10,6 +10,12 @@ fault_manager: # debounce disabled). Set to N to require N reports before CONFIRMED. confirmation_threshold: -1 + # Near-miss series: one appended entry per FAILED report that moved the + # debounce counter without confirming. Retained across clear_fault, bounded + # per fault code, oldest evicted first. 0 = unlimited (grows with the + # reporting rate). + # near_miss.max_per_fault: 200 + # Healing OFF by default: a recovery signal (e.g. action SUCCEEDED) does not # auto-clear the fault until this is enabled. healing_enabled: false 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 09a250070..0f5c91f38 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 @@ -74,6 +74,14 @@ int32_t clamp_debounce_counter(int32_t counter, const DebounceConfig & config); /// callers handle that. This is the single source of truth shared by both storage backends. std::string compute_debounce_status(int32_t counter, const std::string & current_status, const DebounceConfig & config); +/// Whether a just-applied report counts as a near miss: a FAILED report that moved the debounce +/// counter without leaving the fault CONFIRMED - the fault nearly happened. PASSED reports move the +/// counter in the healing direction (the fault receding), so they never qualify. Single source of +/// truth shared by both storage backends. +/// @param is_failed_event Whether the report was FAILED (as opposed to PASSED) +/// @param resulting_status The fault status after the report was applied +bool is_near_miss(bool is_failed_event, const std::string & resulting_status); + /// Validate a (merged) debounce config in place, enforcing confirmation_threshold < 0 <= healing_threshold /// (healing_threshold == 0 means heal on a single PASSED event). Offending fields are reset to safe /// defaults (-1 / 3). Returns true if the config was already valid. @@ -139,6 +147,28 @@ struct FreezeFrameData { /// off the wire with its own copy of this rule; keep the two in step. std::string rosbag_recording_id(const std::string & file_path); +/// One entry of the near-miss series for a fault code. +/// +/// A near miss is a FAILED report that moved the debounce counter WITHOUT the fault +/// ending up CONFIRMED - the fault nearly happened. PASSED reports move the counter +/// too, but in the healing direction (the fault receding), so they are not near misses. +/// This is the debounce sense of the term and is unrelated to any scoring sense used +/// elsewhere in the product. +/// +/// The series is append-only: one entry per qualifying report, never updated in place, +/// and RETAINED across clear_fault, because acknowledging a fault cycle must not erase +/// the record of how often that code approached confirmation. It is bounded per fault +/// code (see set_max_near_misses_per_fault) and evicts the OLDEST entries first, so a +/// long-running appliance keeps the recent series rather than freezing it at boot. +struct NearMissRecord { + std::string fault_code; + int64_t occurred_at_ns{0}; ///< Timestamp of the report that moved the counter + int32_t debounce_counter{0}; ///< Counter value AFTER this report + int32_t confirmation_threshold{0}; ///< Counter value that would have confirmed the fault + uint8_t severity{0}; ///< Severity carried by the report + std::string source_id; ///< Reporting source +}; + /// One row = one LINK: a fault claiming a recording. Several faults of a burst link to /// one recording (same file_path, same recording_id), and one fault can link to several /// recordings over time. Bytes are owned by file_path, not by the row: a bag is unlinked @@ -191,7 +221,9 @@ class FaultStorage { /// @return The fault if found, nullopt otherwise virtual std::optional get_fault(const std::string & fault_code) const = 0; - /// Clear a fault by fault_code (manual acknowledgment) + /// Clear a fault by fault_code (manual acknowledgment). Drops the fault's per-topic snapshots; + /// the freeze-frame and the near-miss series are RETAINED, because they outlive a single fault + /// cycle and cannot be reconstructed afterwards. /// @param fault_code The fault code to clear /// @return true if fault was found and cleared, false if not found virtual bool clear_fault(const std::string & fault_code) = 0; @@ -295,6 +327,17 @@ class FaultStorage { /// codes with no capture configured, which never get a row) virtual std::optional get_freeze_frame(const std::string & fault_code) const = 0; + /// Set the maximum number of near-miss entries retained per fault code. + /// Entries beyond the bound are evicted oldest-first. 0 = unlimited (unbounded growth). + virtual void set_max_near_misses_per_fault(size_t /*max_count*/) { + } + + /// Get the near-miss series for a fault code, oldest entry first. + /// The series survives clear_fault; an unknown or never-near-missed code returns empty. + /// @param fault_code The fault code to look up + /// @return The retained near-miss entries in chronological order + virtual std::vector get_near_misses(const std::string & fault_code) const = 0; + /// Store rosbag file metadata for a fault /// @param info The rosbag file info to store (replaces any existing entry for fault_code) virtual void store_rosbag_file(const RosbagFileInfo & info) = 0; @@ -438,6 +481,10 @@ class InMemoryFaultStorage : public FaultStorage { std::optional get_freeze_frame(const std::string & fault_code) const override; void set_max_rosbags_per_fault(size_t max_count) override; + + void set_max_near_misses_per_fault(size_t max_count) override; + std::vector get_near_misses(const std::string & fault_code) const override; + void store_rosbag_file(const RosbagFileInfo & info) override; /// All-or-nothing, as the base class requires: the batch is built beside the live /// map and swapped in, so a throw leaves the store exactly as it was. @@ -457,6 +504,11 @@ class InMemoryFaultStorage : public FaultStorage { /// Update fault status based on debounce counter and given config void update_status(FaultState & state, const DebounceConfig & config); + /// Append one entry to the near-miss series for @p state and evict the oldest entries beyond + /// max_near_misses_per_fault_. Caller holds mutex_ and has already applied the report to @p state. + void record_near_miss(const FaultState & state, const DebounceConfig & config, uint8_t severity, + const std::string & source_id, const rclcpp::Time & timestamp); + /// Whether a fault other than @p fault_code still references @p file_path. /// One recording can back several faults of the same burst, so the bag must /// only be unlinked once the last of them is gone. Caller holds mutex_. @@ -487,9 +539,11 @@ class InMemoryFaultStorage : public FaultStorage { /// A backend constructed directly (tests, embedders) therefore behaves exactly as /// it always did until someone opts into a history. 0 = unlimited. size_t max_rosbags_per_fault_{1}; + std::map> near_misses_; ///< fault_code -> series (retained across clear) DebounceConfig config_; size_t max_snapshots_per_fault_{0}; ///< 0 = unlimited bool retain_snapshots_on_clear_{false}; + size_t max_near_misses_per_fault_{0}; ///< 0 = unlimited }; } // namespace ros2_medkit_fault_manager 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 e759bc1eb..0d9cc6a86 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 @@ -19,6 +19,7 @@ #include #include #include +#include #include "ros2_medkit_fault_manager/fault_storage.hpp" @@ -77,6 +78,9 @@ class SqliteFaultStorage : public FaultStorage { void store_freeze_frame(const FreezeFrameData & frame) override; std::optional get_freeze_frame(const std::string & fault_code) const override; + void set_max_near_misses_per_fault(size_t max_count) override; + std::vector get_near_misses(const std::string & fault_code) const override; + void store_rosbag_file(const RosbagFileInfo & info) override; void store_rosbag_files(const std::vector & infos) override; std::optional get_rosbag_file(const std::string & fault_code) const override; @@ -130,6 +134,17 @@ class SqliteFaultStorage : public FaultStorage { /// still says nobody holds it. std::vector store_rosbag_file_locked(const RosbagFileInfo & info); + /// Append one entry to the near-miss series and evict the oldest entries beyond + /// max_near_misses_per_fault_. Caller holds mutex_ and has already written the fault row. + /// @param fault_code The fault code that nearly confirmed + /// @param occurred_at_ns Timestamp of the report + /// @param debounce_counter Counter value after the report + /// @param config Debounce config the report was evaluated against + /// @param severity Severity carried by the report + /// @param source_id Reporting source + void record_near_miss_locked(const std::string & fault_code, int64_t occurred_at_ns, int32_t debounce_counter, + const DebounceConfig & config, uint8_t severity, const std::string & source_id); + /// Run a plain SQL statement or throw with the SQLite error. Caller holds mutex_. void exec_or_throw(const char * sql); @@ -143,7 +158,8 @@ class SqliteFaultStorage : public FaultStorage { sqlite3 * db_{nullptr}; mutable std::mutex mutex_; DebounceConfig config_; - size_t max_snapshots_per_fault_{0}; ///< 0 = unlimited + size_t max_snapshots_per_fault_{0}; ///< 0 = unlimited + size_t max_near_misses_per_fault_{0}; ///< 0 = unlimited bool retain_snapshots_on_clear_{false}; /// Defaults to 1, the pre-#620 behaviour: a new recording replaces the old one. /// 0 = unlimited, bounded only by max_total_storage_mb. 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 b1dc5daab..73b5ea186 100644 --- a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp @@ -185,6 +185,19 @@ FaultManagerNode::FaultManagerNode(const rclcpp::NodeOptions & options) : Node(" max_snapshots = 0; } + // Near-miss retention. The series is what any later look at "how often did this code approach + // confirmation" reads, so it is bounded rather than unlimited by default: an appliance runs for + // weeks and the rows are cheap but not free. 0 means unlimited and lets the file grow with the + // reporting rate. declare_parameter deliberately: narrows silently, and a + // negative value cast to size_t becomes SIZE_MAX, which would remove the bound entirely. + constexpr int64_t kDefaultMaxNearMissesPerFault = 200; + auto max_near_misses = declare_parameter("near_miss.max_per_fault", kDefaultMaxNearMissesPerFault); + if (max_near_misses < 0) { + RCLCPP_WARN(get_logger(), "near_miss.max_per_fault must be >= 0, got %ld. Using default %ld.", max_near_misses, + kDefaultMaxNearMissesPerFault); + max_near_misses = kDefaultMaxNearMissesPerFault; + } + // Create storage backend storage_ = create_storage(); @@ -199,6 +212,9 @@ FaultManagerNode::FaultManagerNode(const rclcpp::NodeOptions & options) : Node(" // acknowledgement is a separate question from how many sets are kept. storage_->set_retain_snapshots_on_clear(retain_snapshots_on_clear); + // Apply near-miss retention bound to storage (0 = unlimited) + storage_->set_max_near_misses_per_fault(static_cast(max_near_misses)); + // Create event publisher for SSE streaming event_publisher_ = create_publisher("~/events", rclcpp::QoS(100).reliable()); diff --git a/src/ros2_medkit_fault_manager/src/fault_storage.cpp b/src/ros2_medkit_fault_manager/src/fault_storage.cpp index 689ef9d99..1001e30fe 100644 --- a/src/ros2_medkit_fault_manager/src/fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_storage.cpp @@ -64,6 +64,10 @@ std::string compute_debounce_status(int32_t counter, const std::string & current return current_status; // counter == 0 keeps the current status (avoids flapping at the boundary) } +bool is_near_miss(bool is_failed_event, const std::string & resulting_status) { + return is_failed_event && resulting_status != ros2_medkit_msgs::msg::Fault::STATUS_CONFIRMED; +} + bool sanitize_debounce_config(DebounceConfig & config) { bool valid = true; if (config.confirmation_threshold >= 0) { @@ -148,6 +152,10 @@ bool InMemoryFaultStorage::report_fault_event(const std::string & fault_code, ui update_status(state, config); } + if (is_near_miss(true, state.status)) { + record_near_miss(state, config, severity, source_id, timestamp); + } + faults_.emplace(fault_code, std::move(state)); return true; } @@ -183,6 +191,9 @@ bool InMemoryFaultStorage::report_fault_event(const std::string & fault_code, ui } else { update_status(state, config); } + if (is_near_miss(true, state.status)) { + record_near_miss(state, config, severity, source_id, timestamp); + } return true; // Reactivation treated as new occurrence for event publishing } @@ -232,6 +243,10 @@ bool InMemoryFaultStorage::report_fault_event(const std::string & fault_code, ui // Update status based on debounce counter update_status(state, config); + if (is_near_miss(is_failed, state.status)) { + record_near_miss(state, config, severity, source_id, timestamp); + } + return false; } @@ -474,6 +489,41 @@ std::optional InMemoryFaultStorage::get_freeze_frame(const std: return it->second; } +void InMemoryFaultStorage::record_near_miss(const FaultState & state, const DebounceConfig & config, uint8_t severity, + const std::string & source_id, const rclcpp::Time & timestamp) { + auto & series = near_misses_[state.fault_code]; + + NearMissRecord record; + record.fault_code = state.fault_code; + record.occurred_at_ns = timestamp.nanoseconds(); + record.debounce_counter = state.debounce_counter; + record.confirmation_threshold = config.confirmation_threshold; + record.severity = severity; + record.source_id = source_id; + series.push_back(std::move(record)); + + // Evict oldest-first: a series frozen at boot says nothing about a trend. + if (max_near_misses_per_fault_ > 0 && series.size() > max_near_misses_per_fault_) { + using DiffType = std::vector::difference_type; + const auto excess = static_cast(series.size() - max_near_misses_per_fault_); + series.erase(series.begin(), series.begin() + excess); + } +} + +void InMemoryFaultStorage::set_max_near_misses_per_fault(size_t max_count) { + std::lock_guard lock(mutex_); + max_near_misses_per_fault_ = max_count; +} + +std::vector InMemoryFaultStorage::get_near_misses(const std::string & fault_code) const { + std::lock_guard lock(mutex_); + auto it = near_misses_.find(fault_code); + if (it == near_misses_.end()) { + return {}; + } + return it->second; +} + void InMemoryFaultStorage::set_max_rosbags_per_fault(size_t max_count) { std::lock_guard lock(mutex_); max_rosbags_per_fault_ = max_count; 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 648674aa4..ea723cbf3 100644 --- a/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp @@ -266,6 +266,30 @@ void SqliteFaultStorage::initialize_schema() { } } + // Create near_misses table: append-only series of FAILED reports that moved the debounce + // counter without confirming the fault. One row per qualifying report, never updated in + // place, and NOT removed on clear_fault - acknowledging a fault cycle must not erase how + // often that code approached confirmation. Bounded per fault code by the caller-supplied + // limit, evicting the oldest rows first. + const char * create_near_misses_table_sql = R"( + CREATE TABLE IF NOT EXISTS near_misses ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + fault_code TEXT NOT NULL, + occurred_at_ns INTEGER NOT NULL, + debounce_counter INTEGER NOT NULL, + confirmation_threshold INTEGER NOT NULL, + severity INTEGER NOT NULL, + source_id TEXT NOT NULL + ); + CREATE INDEX IF NOT EXISTS idx_near_misses_fault_code ON near_misses(fault_code, occurred_at_ns, id); + )"; + + if (sqlite3_exec(db_, create_near_misses_table_sql, nullptr, nullptr, &err_msg) != SQLITE_OK) { + std::string error = err_msg ? err_msg : "Unknown error"; + sqlite3_free(err_msg); + throw std::runtime_error("Failed to create near_misses table: " + error); + } + // Create rosbag_files table. One row = one LINK (a fault claiming a recording): // several faults of a burst link to one bag, and one fault links to several bags // over time. Bytes belong to file_path, not to the row. @@ -734,6 +758,10 @@ bool SqliteFaultStorage::report_fault_event(const std::string & fault_code, uint if (update_stmt.step() != SQLITE_DONE) { throw std::runtime_error(std::string("Failed to update fault: ") + sqlite3_errmsg(db_)); } + + if (is_near_miss(true, new_status)) { + record_near_miss_locked(fault_code, timestamp_ns, debounce_counter, config, severity, source_id); + } } else { // PASSED event - increment towards healing, clamped to the thresholds. debounce_counter = clamp_debounce_counter(debounce_counter + 1, config); @@ -797,6 +825,10 @@ bool SqliteFaultStorage::report_fault_event(const std::string & fault_code, uint throw std::runtime_error(std::string("Failed to insert fault: ") + sqlite3_errmsg(db_)); } + if (is_near_miss(true, initial_status)) { + record_near_miss_locked(fault_code, timestamp_ns, initial_counter, config, severity, source_id); + } + return true; // New fault created } @@ -904,6 +936,10 @@ std::optional SqliteFaultStorage::get_fault(const bool SqliteFaultStorage::clear_fault(const std::string & fault_code) { std::lock_guard lock(mutex_); + // The near_misses rows for this code are deliberately left alone. Clearing acknowledges one + // fault cycle; the record of how often the code approached confirmation spans cycles and + // cannot be reconstructed once deleted. + // Delete associated snapshots when fault is cleared // Acknowledging a fault drops its value snapshots, unless a history was asked // for: with recordings retained past a clear, deleting the readings that go with @@ -1207,6 +1243,72 @@ std::optional SqliteFaultStorage::get_freeze_frame(const std::s return frame; } +void SqliteFaultStorage::set_max_near_misses_per_fault(size_t max_count) { + std::lock_guard lock(mutex_); + max_near_misses_per_fault_ = max_count; +} + +void SqliteFaultStorage::record_near_miss_locked(const std::string & fault_code, int64_t occurred_at_ns, + int32_t debounce_counter, const DebounceConfig & config, + uint8_t severity, const std::string & source_id) { + SqliteStatement insert_stmt(db_, + "INSERT INTO near_misses (fault_code, occurred_at_ns, debounce_counter, " + "confirmation_threshold, severity, source_id) VALUES (?, ?, ?, ?, ?, ?)"); + insert_stmt.bind_text(1, fault_code); + insert_stmt.bind_int64(2, occurred_at_ns); + insert_stmt.bind_int(3, debounce_counter); + insert_stmt.bind_int(4, config.confirmation_threshold); + insert_stmt.bind_int(5, static_cast(severity)); + insert_stmt.bind_text(6, source_id); + + if (insert_stmt.step() != SQLITE_DONE) { + throw std::runtime_error(std::string("Failed to store near miss: ") + sqlite3_errmsg(db_)); + } + + if (max_near_misses_per_fault_ == 0) { + return; // Unlimited + } + + // Evict oldest-first, keeping the newest max_near_misses_per_fault_ rows. Newest-first is the + // deliberate opposite of the snapshot limit's keep-earliest rule: a series frozen at boot + // answers nothing about whether the rate of near misses is changing. + SqliteStatement trim_stmt(db_, + "DELETE FROM near_misses WHERE fault_code = ?1 AND id NOT IN " + "(SELECT id FROM near_misses WHERE fault_code = ?1 " + "ORDER BY occurred_at_ns DESC, id DESC LIMIT ?2)"); + trim_stmt.bind_text(1, fault_code); + trim_stmt.bind_int64(2, static_cast(max_near_misses_per_fault_)); + + if (trim_stmt.step() != SQLITE_DONE) { + throw std::runtime_error(std::string("Failed to trim near-miss series: ") + sqlite3_errmsg(db_)); + } +} + +std::vector SqliteFaultStorage::get_near_misses(const std::string & fault_code) const { + std::lock_guard lock(mutex_); + + std::vector result; + + SqliteStatement stmt(db_, + "SELECT fault_code, occurred_at_ns, debounce_counter, confirmation_threshold, " + "severity, source_id FROM near_misses WHERE fault_code = ? " + "ORDER BY occurred_at_ns ASC, id ASC"); + stmt.bind_text(1, fault_code); + + while (stmt.step() == SQLITE_ROW) { + NearMissRecord record; + record.fault_code = stmt.column_text(0); + record.occurred_at_ns = stmt.column_int64(1); + record.debounce_counter = stmt.column_int(2); + record.confirmation_threshold = stmt.column_int(3); + record.severity = static_cast(stmt.column_int(4)); + record.source_id = stmt.column_text(5); + result.push_back(std::move(record)); + } + + return result; +} + void SqliteFaultStorage::exec_or_throw(const char * sql) { char * err_msg = nullptr; if (sqlite3_exec(db_, sql, nullptr, nullptr, &err_msg) != SQLITE_OK) { 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 e2915e933..99e85f983 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -946,6 +946,72 @@ TEST(FaultManagerNodeParameterTest, ClampsInvalidCaptureParams) { EXPECT_EQ(node->capture_queue_full_policy_for_test(), ros2_medkit_fault_manager::QueueFullPolicy::kRejectNewest); } +/// Drive @p count FAILED reports that move the debounce counter without confirming. +/// The threshold sits far below any count used here, so no report in the run confirms the fault. +static void drive_near_misses(ros2_medkit_fault_manager::FaultStorage & storage, int count) { + DebounceConfig config; + config.confirmation_threshold = -1000; + config.critical_immediate_confirm = false; + constexpr int64_t kBaseNs = 1700000000000000000LL; + + for (int i = 0; i < count; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", + rclcpp::Time(kBaseNs + static_cast(i) * 1000000LL), config); + } +} + +TEST(FaultManagerNodeParameterTest, AppliesNearMissRetentionBound) { + rclcpp::NodeOptions options; + options.parameter_overrides({ + {"storage_type", "memory"}, + {"near_miss.max_per_fault", 4}, + }); + auto node = std::make_shared(options); + + drive_near_misses(node->get_storage_for_test(), 10); + + EXPECT_EQ(node->get_storage().get_near_misses("PUMP_PRESSURE_LOW").size(), 4u); +} + +TEST(FaultManagerNodeParameterTest, NearMissRetentionDefaultsToBounded) { + rclcpp::NodeOptions options; + options.parameter_overrides({{"storage_type", "memory"}}); + auto node = std::make_shared(options); + + drive_near_misses(node->get_storage_for_test(), 205); + + // The documented default is 200 per fault code, and it must be in force without configuration. + EXPECT_EQ(node->get_storage().get_near_misses("PUMP_PRESSURE_LOW").size(), 200u); +} + +TEST(FaultManagerNodeParameterTest, NegativeNearMissBoundFallsBackToDefault) { + // A negative value cast straight to size_t becomes SIZE_MAX, silently removing the bound. + rclcpp::NodeOptions options; + options.parameter_overrides({ + {"storage_type", "memory"}, + {"near_miss.max_per_fault", -5}, + }); + auto node = std::make_shared(options); + + drive_near_misses(node->get_storage_for_test(), 205); + + EXPECT_EQ(node->get_storage().get_near_misses("PUMP_PRESSURE_LOW").size(), 200u); +} + +TEST(FaultManagerNodeParameterTest, ZeroNearMissBoundIsUnlimited) { + rclcpp::NodeOptions options; + options.parameter_overrides({ + {"storage_type", "memory"}, + {"near_miss.max_per_fault", 0}, + }); + auto node = std::make_shared(options); + + drive_near_misses(node->get_storage_for_test(), 205); + + EXPECT_EQ(node->get_storage().get_near_misses("PUMP_PRESSURE_LOW").size(), 205u); +} + TEST(FaultManagerNodeParameterTest, ParsesDropOldestPolicy) { rclcpp::NodeOptions options; options.parameter_overrides({ @@ -2404,6 +2470,94 @@ TEST(FaultAuditFailClosedTest, FailClosedAbortsAndFlags) { remove_audit_files(audit_path); } +// --- InMemoryFaultStorage near-miss series tests --- +// +// The in-memory backend must keep the same near-miss contract as SQLite: appended per +// qualifying report, bounded oldest-first, retained across clear_fault. + +/// Debounce config that takes four FAILED reports to confirm, leaving three near misses first. +static DebounceConfig near_miss_config() { + DebounceConfig config; + config.confirmation_threshold = -4; + config.critical_immediate_confirm = false; + return config; +} + +static rclcpp::Time near_miss_time(int index) { + constexpr int64_t kBaseNs = 1700000000000000000LL; + return rclcpp::Time(kBaseNs + static_cast(index) * 1000000LL); +} + +TEST(InMemoryNearMissTest, SeriesIsAppendedAndSurvivesClear) { + InMemoryFaultStorage storage; + const auto config = near_miss_config(); + + for (int i = 0; i < 3; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(i), config); + } + + auto series = storage.get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 3u); + EXPECT_EQ(series[0].debounce_counter, -1); + EXPECT_EQ(series[2].debounce_counter, -3); + EXPECT_EQ(series[0].confirmation_threshold, -4); + + ASSERT_TRUE(storage.clear_fault("PUMP_PRESSURE_LOW")); + EXPECT_EQ(storage.get_near_misses("PUMP_PRESSURE_LOW").size(), 3u); +} + +TEST(InMemoryNearMissTest, ConfirmingReportIsNotANearMiss) { + InMemoryFaultStorage storage; + const auto config = near_miss_config(); + + for (int i = 0; i < 4; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(i), config); + } + + auto fault = storage.get_fault("PUMP_PRESSURE_LOW"); + ASSERT_TRUE(fault.has_value()); + ASSERT_EQ(fault->status, Fault::STATUS_CONFIRMED); + EXPECT_EQ(storage.get_near_misses("PUMP_PRESSURE_LOW").size(), 3u); +} + +TEST(InMemoryNearMissTest, SeriesBoundedKeepingNewest) { + InMemoryFaultStorage storage; + DebounceConfig config = near_miss_config(); + config.confirmation_threshold = -20; + storage.set_max_near_misses_per_fault(3); + + for (int i = 0; i < 5; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(i), config); + } + + auto series = storage.get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 3u); + EXPECT_EQ(series[0].debounce_counter, -3); + EXPECT_EQ(series[2].debounce_counter, -5); +} + +TEST(InMemoryNearMissTest, PassedReportIsNotANearMiss) { + InMemoryFaultStorage storage; + const auto config = near_miss_config(); + + for (int i = 0; i < 2; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(i), config); + } + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_WARN, "", + "/hydraulics/pump", near_miss_time(2), config); + + EXPECT_EQ(storage.get_near_misses("PUMP_PRESSURE_LOW").size(), 2u); +} + +TEST(InMemoryNearMissTest, EmptyForUnknownFault) { + InMemoryFaultStorage storage; + EXPECT_TRUE(storage.get_near_misses("NEVER_REPORTED").empty()); +} + int main(int argc, char ** argv) { rclcpp::init(argc, argv); ::testing::InitGoogleTest(&argc, argv); 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 e1b508ce9..576bab4e9 100644 --- a/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp +++ b/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp @@ -1799,6 +1799,302 @@ TEST_F(SqliteFaultStorageTest, SnapshotLimitPerFaultNotGlobal) { EXPECT_EQ(storage_->get_snapshots("FAULT_B").size(), 1u); } +// --- Near-miss series --- +// +// A near miss is a FAILED report that moved the debounce counter without the fault ending up +// CONFIRMED. The series is append-only and must survive clear_fault, because acknowledging a +// fault cycle must not erase how often that code approached confirmation. + +/// Debounce config that takes four FAILED reports to confirm, leaving three near misses first. +static DebounceConfig four_strike_config() { + DebounceConfig config; + config.confirmation_threshold = -4; + config.critical_immediate_confirm = false; + return config; +} + +/// Deterministic timestamps 1 ms apart, so series ordering is checkable. +static rclcpp::Time nth_report_time(int index) { + constexpr int64_t kBaseNs = 1700000000000000000LL; + return rclcpp::Time(kBaseNs + static_cast(index) * 1000000LL); +} + +TEST_F(SqliteFaultStorageTest, NearMissSeriesIsAppendedNotOverwritten) { + const auto config = four_strike_config(); + + for (int i = 0; i < 3; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + + auto fault = storage_->get_fault("PUMP_PRESSURE_LOW"); + ASSERT_TRUE(fault.has_value()); + ASSERT_NE(fault->status, Fault::STATUS_CONFIRMED) << "test setup: these reports must not confirm"; + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 3u) << "each near miss must append an entry, not overwrite the last"; + EXPECT_EQ(series[0].debounce_counter, -1); + EXPECT_EQ(series[1].debounce_counter, -2); + EXPECT_EQ(series[2].debounce_counter, -3); + EXPECT_EQ(series[0].confirmation_threshold, -4); + EXPECT_EQ(series[0].fault_code, "PUMP_PRESSURE_LOW"); + EXPECT_EQ(series[0].source_id, "/hydraulics/pump"); + EXPECT_EQ(series[0].severity, Fault::SEVERITY_WARN); + EXPECT_EQ(series[0].occurred_at_ns, nth_report_time(0).nanoseconds()); + EXPECT_LT(series[0].occurred_at_ns, series[2].occurred_at_ns); +} + +TEST_F(SqliteFaultStorageTest, ConfirmingReportIsNotANearMiss) { + const auto config = four_strike_config(); + + for (int i = 0; i < 4; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + + auto fault = storage_->get_fault("PUMP_PRESSURE_LOW"); + ASSERT_TRUE(fault.has_value()); + ASSERT_EQ(fault->status, Fault::STATUS_CONFIRMED); + + // The fourth report is the fault happening, not nearly happening. + EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 3u); +} + +TEST_F(SqliteFaultStorageTest, NearMissSeriesSurvivesClearFault) { + const auto config = four_strike_config(); + + for (int i = 0; i < 4; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + ASSERT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 3u); + + ASSERT_TRUE(storage_->clear_fault("PUMP_PRESSURE_LOW")); + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 3u) << "acknowledging the fault destroyed the near-miss record"; + EXPECT_EQ(series[0].debounce_counter, -1); + EXPECT_EQ(series[2].debounce_counter, -3); +} + +TEST_F(SqliteFaultStorageTest, NearMissSeriesContinuesAcrossReactivation) { + const auto config = four_strike_config(); + + for (int i = 0; i < 4; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + ASSERT_TRUE(storage_->clear_fault("PUMP_PRESSURE_LOW")); + + // A new outage cycle starts: the reactivating report resets the counter to -1 without confirming. + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping again", "/hydraulics/pump", nth_report_time(10), config); + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 4u) << "the series must span fault cycles, one entry per occurrence"; + EXPECT_EQ(series[3].debounce_counter, -1); + EXPECT_EQ(series[3].occurred_at_ns, nth_report_time(10).nanoseconds()); +} + +TEST_F(SqliteFaultStorageTest, NearMissSeriesSurvivesReopen) { + const auto config = four_strike_config(); + + for (int i = 0; i < 3; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + ASSERT_TRUE(storage_->clear_fault("PUMP_PRESSURE_LOW")); + + storage_.reset(); + storage_ = std::make_unique(temp_db_path_.string()); + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 3u) << "the series must outlive the process, not just the fault cycle"; + EXPECT_EQ(series[0].debounce_counter, -1); + EXPECT_EQ(series[2].debounce_counter, -3); +} + +TEST_F(SqliteFaultStorageTest, PassedReportIsNotANearMiss) { + const auto config = four_strike_config(); + + for (int i = 0; i < 2; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + ASSERT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 2u); + + // A PASSED report moves the counter in the healing direction: the fault receding, not nearing. + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_WARN, "", + "/hydraulics/pump", nth_report_time(2), config); + + EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 2u); +} + +TEST_F(SqliteFaultStorageTest, CriticalImmediateConfirmIsNotANearMiss) { + DebounceConfig config = four_strike_config(); + config.critical_immediate_confirm = true; + + storage_->report_fault_event("BATTERY_THERMAL_RUNAWAY", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_CRITICAL, + "cell over temperature", "/power/bms", nth_report_time(0), config); + + auto fault = storage_->get_fault("BATTERY_THERMAL_RUNAWAY"); + ASSERT_TRUE(fault.has_value()); + ASSERT_EQ(fault->status, Fault::STATUS_CONFIRMED); + EXPECT_TRUE(storage_->get_near_misses("BATTERY_THERMAL_RUNAWAY").empty()); +} + +TEST_F(SqliteFaultStorageTest, ImmediateConfirmThresholdRecordsNoNearMiss) { + // Endpoint of the documented range: confirmation_threshold = -1 confirms on the first report, + // so a fault under this config never has a near miss to record. + const auto config = default_config(); + + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, + "pressure lost", "/hydraulics/pump", nth_report_time(0), config); + + auto fault = storage_->get_fault("PUMP_PRESSURE_LOW"); + ASSERT_TRUE(fault.has_value()); + ASSERT_EQ(fault->status, Fault::STATUS_CONFIRMED); + EXPECT_TRUE(storage_->get_near_misses("PUMP_PRESSURE_LOW").empty()); +} + +TEST_F(SqliteFaultStorageTest, FailedReportUnderHealedLatchIsNearMiss) { + DebounceConfig config = four_strike_config(); + config.healing_enabled = true; + config.healing_threshold = 1; + + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(0), config); + ASSERT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 1u); + + for (int i = 1; i <= 2; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_WARN, "", + "/hydraulics/pump", nth_report_time(i), config); + } + auto healed = storage_->get_fault("PUMP_PRESSURE_LOW"); + ASSERT_TRUE(healed.has_value()); + ASSERT_EQ(healed->status, Fault::STATUS_HEALED) << "test setup: the fault must be latched HEALED"; + + // The latch keeps the status at HEALED, but the counter moved toward confirmation: a near miss. + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(3), config); + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 2u); + EXPECT_EQ(series[1].debounce_counter, 0); +} + +TEST_F(SqliteFaultStorageTest, NearMissSeriesBoundedKeepingNewest) { + DebounceConfig config = four_strike_config(); + config.confirmation_threshold = -20; + storage_->set_max_near_misses_per_fault(3); + + for (int i = 0; i < 5; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 3u); + // Oldest-first eviction: a series frozen at boot would say nothing about a trend. + EXPECT_EQ(series[0].debounce_counter, -3); + EXPECT_EQ(series[1].debounce_counter, -4); + EXPECT_EQ(series[2].debounce_counter, -5); +} + +TEST_F(SqliteFaultStorageTest, NearMissBoundOfOneKeepsLatestOnly) { + DebounceConfig config = four_strike_config(); + config.confirmation_threshold = -20; + storage_->set_max_near_misses_per_fault(1); + + for (int i = 0; i < 4; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 1u); + EXPECT_EQ(series[0].debounce_counter, -4); +} + +TEST_F(SqliteFaultStorageTest, NearMissBoundIsPerFaultCode) { + DebounceConfig config = four_strike_config(); + config.confirmation_threshold = -20; + storage_->set_max_near_misses_per_fault(2); + + for (int i = 0; i < 3; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + storage_->report_fault_event("MOTOR_OVERHEAT", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "temperature rising", "/powertrain/motor", nth_report_time(i), config); + } + + EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 2u); + EXPECT_EQ(storage_->get_near_misses("MOTOR_OVERHEAT").size(), 2u); +} + +TEST_F(SqliteFaultStorageTest, NearMissBoundZeroIsUnlimited) { + DebounceConfig config = four_strike_config(); + config.confirmation_threshold = -200; + storage_->set_max_near_misses_per_fault(0); + + for (int i = 0; i < 150; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + + EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 150u); +} + +TEST_F(SqliteFaultStorageTest, NearMissTableCreatedOnDatabaseFromOlderBuild) { + // The appliances this matters for already have a faults.db written by a build with no + // near_misses table. Opening one must add the table, not fail and not skip recording. + const auto config = four_strike_config(); + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(0), config); + storage_.reset(); + + { + sqlite3 * raw = nullptr; + ASSERT_EQ(sqlite3_open(temp_db_path_.string().c_str(), &raw), SQLITE_OK); + ASSERT_EQ(sqlite3_exec(raw, "DROP TABLE near_misses", nullptr, nullptr, nullptr), SQLITE_OK); + sqlite3_close(raw); + } + + storage_ = std::make_unique(temp_db_path_.string()); + EXPECT_TRUE(storage_->get_near_misses("PUMP_PRESSURE_LOW").empty()); + + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(1), config); + EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 1u); +} + +TEST_F(SqliteFaultStorageTest, NearMissSeriesSurvivesHealedReclassification) { + // Startup reclassification is the other place that drops a fault's captured data; it must + // leave the series alone for the same reason clear_fault does. + DebounceConfig config = four_strike_config(); + config.healing_enabled = true; + config.healing_threshold = 1; + + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(0), config); + for (int i = 1; i <= 2; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_WARN, "", + "/hydraulics/pump", nth_report_time(i), config); + } + auto healed = storage_->get_fault("PUMP_PRESSURE_LOW"); + ASSERT_TRUE(healed.has_value()); + ASSERT_EQ(healed->status, Fault::STATUS_HEALED) << "test setup: the fault must be latched HEALED"; + ASSERT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 1u); + + ASSERT_EQ(storage_->reclassify_healed_as_cleared().size(), 1u); + + EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 1u); +} + +TEST_F(SqliteFaultStorageTest, NearMissSeriesEmptyForUnknownFault) { + EXPECT_TRUE(storage_->get_near_misses("NEVER_REPORTED").empty()); +} + int main(int argc, char ** argv) { rclcpp::init(argc, argv); ::testing::InitGoogleTest(&argc, argv); From 16b666420bb7462927e4c26c29d910047d06f399 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 20 Aug 2026 18:28:35 +0200 Subject: [PATCH 2/9] fix(fault_manager): correct near-miss eviction order, bound application and atomicity Three defects in the near-miss series. The SQLite backend evicted by event timestamp while the in-memory backend evicted by arrival. Reporters carry their own clocks, so a report can arrive with a timestamp behind one already stored. SQLite then deleted the row it had just written, and the two backends returned different histories for the same input. Both now keep and evict in arrival order. Setting the retention bound did not trim what was already stored. A database that grew under a larger bound, or none, stayed over the new bound until each fault code happened to record another near miss, and a code that went quiet kept its rows for good. Applying the bound now trims immediately, per fault code. The fault row write and the near-miss append were separate autocommit statements. A failure on the append left the debounce counter already advanced, so the caller's retry advanced it a second time while the near miss it retried for stayed missing. They now commit together. Two documentation statements still said snapshots are always deleted when a fault is cleared, which stopped being true when the retention switch was added. --- src/ros2_medkit_fault_manager/README.md | 14 ++- .../sqlite_fault_storage.hpp | 6 + .../src/fault_storage.cpp | 15 +++ .../src/sqlite_fault_storage.cpp | 46 +++++++- .../test/test_fault_manager.cpp | 72 ++++++++++++ .../test/test_sqlite_storage.cpp | 107 ++++++++++++++++++ 6 files changed, 254 insertions(+), 6 deletions(-) diff --git a/src/ros2_medkit_fault_manager/README.md b/src/ros2_medkit_fault_manager/README.md index 914b3b0e2..bea6d7c46 100644 --- a/src/ros2_medkit_fault_manager/README.md +++ b/src/ros2_medkit_fault_manager/README.md @@ -143,13 +143,19 @@ direction (the fault receding), so they are not near misses. The series is **append-only**: one entry per qualifying report, holding the timestamp, the counter value after the report, the confirmation threshold it was measured against, the severity and the -reporting source. Nothing is updated in place, so "this code approached confirmation five times -this hour" stays answerable. +reporting source. Every field describes that one report, so a series spanning a threshold change +or a new reporting source stays readable. Nothing is updated in place, so "this code approached +confirmation five times this hour" stays answerable. + +Entries are kept and evicted in **arrival order**, not by their timestamps. Reporters carry their +own clocks, so a report can arrive carrying a timestamp behind one already stored; ordering the +series by timestamp would let such a report evict itself on arrival. It is **retained across `~/clear_fault`**. Clearing acknowledges one fault cycle; how often a code approaches confirmation spans cycles, and once deleted it cannot be reconstructed. The same holds -for the startup reclassification of HEALED faults. Per-topic snapshots are still dropped on clear - -they belong to the one confirmed occurrence, not to the series. +for the startup reclassification of HEALED faults. Per-topic snapshots are dropped on clear by +default, because they belong to the one confirmed occurrence rather than to the series; set +`snapshots.retain_on_clear` to keep them as well. Retention is **bounded per fault code** by `near_miss.max_per_fault` (default 200), evicting the **oldest** entries first. That is deliberately the opposite of the snapshot limit's keep-earliest 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 0d9cc6a86..75230f059 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 @@ -134,6 +134,12 @@ class SqliteFaultStorage : public FaultStorage { /// still says nobody holds it. std::vector store_rosbag_file_locked(const RosbagFileInfo & info); + /// report_fault_event body without taking mutex_ or opening a transaction. Caller holds mutex_ + /// and wraps the call, so the fault row and any near-miss row commit together. + bool report_fault_event_locked(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); + /// Append one entry to the near-miss series and evict the oldest entries beyond /// max_near_misses_per_fault_. Caller holds mutex_ and has already written the fault row. /// @param fault_code The fault code that nearly confirmed diff --git a/src/ros2_medkit_fault_manager/src/fault_storage.cpp b/src/ros2_medkit_fault_manager/src/fault_storage.cpp index 1001e30fe..bbe543279 100644 --- a/src/ros2_medkit_fault_manager/src/fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_storage.cpp @@ -513,6 +513,21 @@ void InMemoryFaultStorage::record_near_miss(const FaultState & state, const Debo void InMemoryFaultStorage::set_max_near_misses_per_fault(size_t max_count) { std::lock_guard lock(mutex_); max_near_misses_per_fault_ = max_count; + + if (max_count == 0) { + return; // Unlimited + } + + // Apply the bound to what is already stored, so a series built under a larger bound does not + // stay over the new one until the next near miss for that code happens to arrive. + using DiffType = std::vector::difference_type; + for (auto & [code, series] : near_misses_) { + (void)code; + if (series.size() > max_count) { + const auto excess = static_cast(series.size() - max_count); + series.erase(series.begin(), series.begin() + excess); + } + } } std::vector InMemoryFaultStorage::get_near_misses(const std::string & fault_code) const { 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 ea723cbf3..a5d5f1788 100644 --- a/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp @@ -642,6 +642,25 @@ bool SqliteFaultStorage::report_fault_event(const std::string & fault_code, uint const rclcpp::Time & timestamp, const DebounceConfig & config) { std::lock_guard lock(mutex_); + // The fault row and the near-miss row have to land together. Written as separate autocommit + // statements, a failure on the second would leave the debounce counter already advanced, so the + // caller's retry would advance it a second time and the near miss it retried for would still be + // missing from the series. + exec_or_throw("BEGIN IMMEDIATE"); + try { + const bool is_new_occurrence = + report_fault_event_locked(fault_code, event_type, severity, description, source_id, timestamp, config); + exec_or_throw("COMMIT"); + return is_new_occurrence; + } catch (...) { + sqlite3_exec(db_, "ROLLBACK", nullptr, nullptr, nullptr); + throw; + } +} + +bool SqliteFaultStorage::report_fault_event_locked(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) { int64_t timestamp_ns = timestamp.nanoseconds(); const bool is_failed = (event_type == EventType::EVENT_FAILED); @@ -1246,6 +1265,24 @@ std::optional SqliteFaultStorage::get_freeze_frame(const std::s void SqliteFaultStorage::set_max_near_misses_per_fault(size_t max_count) { std::lock_guard lock(mutex_); max_near_misses_per_fault_ = max_count; + + if (max_count == 0) { + return; // Unlimited + } + + // Apply the bound to what is already in the database. Without this, a database that grew under + // a larger bound (or none) stays over the new bound until each fault code happens to record + // another near miss - and a code that never does keeps its rows for good. + SqliteStatement trim_stmt(db_, + "DELETE FROM near_misses WHERE id IN (" + "SELECT id FROM (SELECT id, ROW_NUMBER() OVER " + "(PARTITION BY fault_code ORDER BY id DESC) AS rn FROM near_misses) " + "WHERE rn > ?)"); + trim_stmt.bind_int64(1, static_cast(max_count)); + + if (trim_stmt.step() != SQLITE_DONE) { + throw std::runtime_error(std::string("Failed to apply near-miss bound: ") + sqlite3_errmsg(db_)); + } } void SqliteFaultStorage::record_near_miss_locked(const std::string & fault_code, int64_t occurred_at_ns, @@ -1272,10 +1309,15 @@ void SqliteFaultStorage::record_near_miss_locked(const std::string & fault_code, // Evict oldest-first, keeping the newest max_near_misses_per_fault_ rows. Newest-first is the // deliberate opposite of the snapshot limit's keep-earliest rule: a series frozen at boot // answers nothing about whether the rate of near misses is changing. + // + // "Oldest" means earliest ARRIVAL (id), not earliest occurred_at_ns. Reporters carry their own + // clocks, so a report can arrive with a timestamp behind one already stored; ordering eviction + // by timestamp would then drop the row that was just appended and make the two backends, which + // append in arrival order, disagree on the same input. SqliteStatement trim_stmt(db_, "DELETE FROM near_misses WHERE fault_code = ?1 AND id NOT IN " "(SELECT id FROM near_misses WHERE fault_code = ?1 " - "ORDER BY occurred_at_ns DESC, id DESC LIMIT ?2)"); + "ORDER BY id DESC LIMIT ?2)"); trim_stmt.bind_text(1, fault_code); trim_stmt.bind_int64(2, static_cast(max_near_misses_per_fault_)); @@ -1292,7 +1334,7 @@ std::vector SqliteFaultStorage::get_near_misses(const std::strin SqliteStatement stmt(db_, "SELECT fault_code, occurred_at_ns, debounce_counter, confirmation_threshold, " "severity, source_id FROM near_misses WHERE fault_code = ? " - "ORDER BY occurred_at_ns ASC, id ASC"); + "ORDER BY id ASC"); stmt.bind_text(1, fault_code); while (stmt.step() == SQLITE_ROW) { 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 99e85f983..d68f28fd0 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -2553,6 +2553,78 @@ TEST(InMemoryNearMissTest, PassedReportIsNotANearMiss) { EXPECT_EQ(storage.get_near_misses("PUMP_PRESSURE_LOW").size(), 2u); } +TEST(InMemoryNearMissTest, SeriesSurvivesHealedReclassification) { + InMemoryFaultStorage storage; + DebounceConfig config = near_miss_config(); + config.healing_enabled = true; + config.healing_threshold = 1; + + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(0), config); + for (int i = 1; i <= 2; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_WARN, "", + "/hydraulics/pump", near_miss_time(i), config); + } + auto healed = storage.get_fault("PUMP_PRESSURE_LOW"); + ASSERT_TRUE(healed.has_value()); + ASSERT_EQ(healed->status, Fault::STATUS_HEALED) << "test setup: the fault must be latched HEALED"; + ASSERT_EQ(storage.get_near_misses("PUMP_PRESSURE_LOW").size(), 1u); + + ASSERT_EQ(storage.reclassify_healed_as_cleared().size(), 1u); + + EXPECT_EQ(storage.get_near_misses("PUMP_PRESSURE_LOW").size(), 1u); +} + +TEST(InMemoryNearMissTest, BoundIsPerFaultCode) { + InMemoryFaultStorage storage; + DebounceConfig config = near_miss_config(); + config.confirmation_threshold = -20; + storage.set_max_near_misses_per_fault(2); + + for (int i = 0; i < 3; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(i), config); + storage.report_fault_event("MOTOR_OVERHEAT", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "temperature rising", "/powertrain/motor", near_miss_time(i), config); + } + + EXPECT_EQ(storage.get_near_misses("PUMP_PRESSURE_LOW").size(), 2u); + EXPECT_EQ(storage.get_near_misses("MOTOR_OVERHEAT").size(), 2u); +} + +TEST(InMemoryNearMissTest, BoundZeroIsUnlimited) { + InMemoryFaultStorage storage; + DebounceConfig config = near_miss_config(); + config.confirmation_threshold = -200; + storage.set_max_near_misses_per_fault(0); + + for (int i = 0; i < 150; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(i), config); + } + + EXPECT_EQ(storage.get_near_misses("PUMP_PRESSURE_LOW").size(), 150u); +} + +TEST(InMemoryNearMissTest, ApplyingSmallerBoundTrimsExistingSeries) { + InMemoryFaultStorage storage; + DebounceConfig config = near_miss_config(); + config.confirmation_threshold = -20; + storage.set_max_near_misses_per_fault(0); + + for (int i = 0; i < 5; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(i), config); + } + + storage.set_max_near_misses_per_fault(2); + + auto series = storage.get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 2u); + EXPECT_EQ(series[0].debounce_counter, -4); + EXPECT_EQ(series[1].debounce_counter, -5); +} + TEST(InMemoryNearMissTest, EmptyForUnknownFault) { InMemoryFaultStorage storage; EXPECT_TRUE(storage.get_near_misses("NEVER_REPORTED").empty()); 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 576bab4e9..1ec7c5777 100644 --- a/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp +++ b/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp @@ -2091,6 +2091,113 @@ TEST_F(SqliteFaultStorageTest, NearMissSeriesSurvivesHealedReclassification) { EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 1u); } +TEST_F(SqliteFaultStorageTest, NearMissSeriesUsesArrivalOrderNotTimestamps) { + // Reporters carry their own clocks, so a report can arrive with a timestamp behind one already + // stored. Ordering eviction by timestamp would delete the row that was just appended. + DebounceConfig config = four_strike_config(); + config.confirmation_threshold = -20; + storage_->set_max_near_misses_per_fault(2); + + for (int i = 0; i < 2; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(10 + i), config); + } + // Arrives third, but carries the earliest timestamp of the three. + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(0), config); + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 2u); + EXPECT_EQ(series[0].occurred_at_ns, nth_report_time(11).nanoseconds()); + EXPECT_EQ(series[1].occurred_at_ns, nth_report_time(0).nanoseconds()) + << "the report that just arrived must not be the one evicted"; + EXPECT_EQ(series[1].debounce_counter, -3); +} + +TEST_F(SqliteFaultStorageTest, NearMissEntriesDescribeTheirOwnReport) { + // Each entry must describe the report that produced it, not the fault's current state, or a + // series spanning a threshold change or a new reporting source reads as if nothing changed. + DebounceConfig first = four_strike_config(); + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(0), first); + + DebounceConfig second = four_strike_config(); + second.confirmation_threshold = -8; + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, + "pressure dipping", "/hydraulics/backup_pump", nth_report_time(1), second); + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 2u); + EXPECT_EQ(series[0].confirmation_threshold, -4); + EXPECT_EQ(series[0].severity, Fault::SEVERITY_WARN); + EXPECT_EQ(series[0].source_id, "/hydraulics/pump"); + EXPECT_EQ(series[1].confirmation_threshold, -8); + EXPECT_EQ(series[1].severity, Fault::SEVERITY_ERROR); + EXPECT_EQ(series[1].source_id, "/hydraulics/backup_pump"); +} + +TEST_F(SqliteFaultStorageTest, NearMissSeriesContinuesAfterReopen) { + const auto config = four_strike_config(); + + for (int i = 0; i < 2; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + + storage_.reset(); + storage_ = std::make_unique(temp_db_path_.string()); + + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(2), config); + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 3u) << "a restart must extend the series, not restart or reorder it"; + EXPECT_EQ(series[0].debounce_counter, -1); + EXPECT_EQ(series[1].debounce_counter, -2); + EXPECT_EQ(series[2].debounce_counter, -3); + EXPECT_EQ(series[2].occurred_at_ns, nth_report_time(2).nanoseconds()); +} + +TEST_F(SqliteFaultStorageTest, ApplyingSmallerBoundTrimsExistingSeries) { + // A database that grew under a larger bound, or none, must come back inside the bound as soon + // as it is applied. Waiting for the next near miss leaves a quiet fault code over the bound for + // good. + DebounceConfig config = four_strike_config(); + config.confirmation_threshold = -20; + storage_->set_max_near_misses_per_fault(0); + + for (int i = 0; i < 5; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + storage_->report_fault_event("MOTOR_OVERHEAT", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "temperature rising", "/powertrain/motor", nth_report_time(i), config); + } + ASSERT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 5u); + + storage_->set_max_near_misses_per_fault(2); + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 2u) << "applying the bound left the stored series over it"; + EXPECT_EQ(series[0].debounce_counter, -4); + EXPECT_EQ(series[1].debounce_counter, -5); + EXPECT_EQ(storage_->get_near_misses("MOTOR_OVERHEAT").size(), 2u) << "the bound is applied per fault code"; +} + +TEST_F(SqliteFaultStorageTest, ApplyingUnlimitedBoundKeepsExistingSeries) { + DebounceConfig config = four_strike_config(); + config.confirmation_threshold = -20; + storage_->set_max_near_misses_per_fault(4); + + for (int i = 0; i < 4; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + + storage_->set_max_near_misses_per_fault(0); + + EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 4u); +} + TEST_F(SqliteFaultStorageTest, NearMissSeriesEmptyForUnknownFault) { EXPECT_TRUE(storage_->get_near_misses("NEVER_REPORTED").empty()); } From 399a871647f2faab9770a5826c5b7bd1f9970462 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 20 Aug 2026 18:49:13 +0200 Subject: [PATCH 3/9] fix(fault_manager): bound application, index and backend parity for near misses The near_misses index was still on (fault_code, occurred_at_ns, id) after the series moved to arrival order, so neither the read nor the trim could use it. It is now (fault_code, id). set_max_near_misses_per_fault(SIZE_MAX) bound to int64 as -1, and every row then compared as beyond the bound, so the idiomatic spelling of "no limit" emptied the table. Any bound past what SQLite can express now means unlimited, on the per-report trim as well. Applying a bound trims what is already stored, which deletes history that cannot be recovered. A mistyped parameter did that at boot with nothing said. The setter now returns how many entries it evicted and the node warns. report_fault_event took BEGIN IMMEDIATE for every report, including PASSED reports that write nothing. That made a heal heartbeat contend for the writer lock and fail with SQLITE_BUSY where before it could not. Only FAILED reports, the only ones that can write a second row, take the transaction now. InMemoryFaultStorage::reclassify_healed_as_cleared never dropped snapshots, while the SQLite backend did, so the two answered a snapshot query differently for the same calls. It now follows the same rule and the same retain switch. --- .../fault_storage.hpp | 14 +- .../sqlite_fault_storage.hpp | 2 +- .../src/fault_manager_node.cpp | 11 +- .../src/fault_storage.cpp | 24 ++- .../src/sqlite_fault_storage.cpp | 36 +++-- .../test/test_fault_manager.cpp | 140 ++++++++++++++++++ .../test/test_sqlite_storage.cpp | 47 ++++++ 7 files changed, 255 insertions(+), 19 deletions(-) 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 0f5c91f38..16e2bca49 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 @@ -328,8 +328,16 @@ class FaultStorage { virtual std::optional get_freeze_frame(const std::string & fault_code) const = 0; /// Set the maximum number of near-miss entries retained per fault code. - /// Entries beyond the bound are evicted oldest-first. 0 = unlimited (unbounded growth). - virtual void set_max_near_misses_per_fault(size_t /*max_count*/) { + /// + /// Entries beyond the bound are evicted oldest-first, including entries already stored when the + /// bound is applied. 0 means unlimited, and so does any bound larger than the storage backend + /// can express. + /// + /// @return How many already-stored entries this call evicted. A bound applied by mistake + /// deletes history that cannot be recovered, and the storage layer has no logger, so + /// the caller is the one that can report it. + virtual size_t set_max_near_misses_per_fault(size_t /*max_count*/) { + return 0; } /// Get the near-miss series for a fault code, oldest entry first. @@ -482,7 +490,7 @@ class InMemoryFaultStorage : public FaultStorage { void set_max_rosbags_per_fault(size_t max_count) override; - void set_max_near_misses_per_fault(size_t max_count) override; + size_t set_max_near_misses_per_fault(size_t max_count) override; std::vector get_near_misses(const std::string & fault_code) const override; void store_rosbag_file(const RosbagFileInfo & info) 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 75230f059..2cd4909b6 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 @@ -78,7 +78,7 @@ class SqliteFaultStorage : public FaultStorage { void store_freeze_frame(const FreezeFrameData & frame) override; std::optional get_freeze_frame(const std::string & fault_code) const override; - void set_max_near_misses_per_fault(size_t max_count) override; + size_t set_max_near_misses_per_fault(size_t max_count) override; std::vector get_near_misses(const std::string & fault_code) const override; void store_rosbag_file(const RosbagFileInfo & info) 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 73b5ea186..c23eb9cc8 100644 --- a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp @@ -212,8 +212,15 @@ FaultManagerNode::FaultManagerNode(const rclcpp::NodeOptions & options) : Node(" // acknowledgement is a separate question from how many sets are kept. storage_->set_retain_snapshots_on_clear(retain_snapshots_on_clear); - // Apply near-miss retention bound to storage (0 = unlimited) - storage_->set_max_near_misses_per_fault(static_cast(max_near_misses)); + // Apply near-miss retention bound to storage (0 = unlimited). Applying it also trims a series + // that a previous run left over the bound, which deletes history for good, so say when it does. + const size_t evicted_near_misses = storage_->set_max_near_misses_per_fault(static_cast(max_near_misses)); + if (evicted_near_misses > 0) { + RCLCPP_WARN(get_logger(), + "near_miss.max_per_fault=%ld dropped %zu stored near-miss entries that exceeded the bound. " + "Raise the bound or set it to 0 before restarting if that history was needed.", + max_near_misses, evicted_near_misses); + } // Create event publisher for SSE streaming event_publisher_ = create_publisher("~/events", rclcpp::QoS(100).reliable()); diff --git a/src/ros2_medkit_fault_manager/src/fault_storage.cpp b/src/ros2_medkit_fault_manager/src/fault_storage.cpp index bbe543279..d4e459d8f 100644 --- a/src/ros2_medkit_fault_manager/src/fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_storage.cpp @@ -510,24 +510,27 @@ void InMemoryFaultStorage::record_near_miss(const FaultState & state, const Debo } } -void InMemoryFaultStorage::set_max_near_misses_per_fault(size_t max_count) { +size_t InMemoryFaultStorage::set_max_near_misses_per_fault(size_t max_count) { std::lock_guard lock(mutex_); max_near_misses_per_fault_ = max_count; if (max_count == 0) { - return; // Unlimited + return 0; // Unlimited } // Apply the bound to what is already stored, so a series built under a larger bound does not // stay over the new one until the next near miss for that code happens to arrive. using DiffType = std::vector::difference_type; + size_t evicted = 0; for (auto & [code, series] : near_misses_) { (void)code; if (series.size() > max_count) { - const auto excess = static_cast(series.size() - max_count); - series.erase(series.begin(), series.begin() + excess); + const size_t excess = series.size() - max_count; + series.erase(series.begin(), series.begin() + static_cast(excess)); + evicted += excess; } } + return evicted; } std::vector InMemoryFaultStorage::get_near_misses(const std::string & fault_code) const { @@ -848,6 +851,19 @@ std::vector InMemoryFaultStorage::reclassify_healed_as_cleared() { reclassified.push_back(code); } } + + // Reclassified rows must match CLEARED semantics, snapshots included - the SQLite backend drops + // them here, and a backend that kept them would answer a snapshot query differently for the + // same sequence of calls. The near-miss series is retained, as it is on clear_fault. + if (!retain_snapshots_on_clear_ && !reclassified.empty()) { + const std::set affected(reclassified.begin(), reclassified.end()); + snapshots_.erase(std::remove_if(snapshots_.begin(), snapshots_.end(), + [&affected](const SnapshotData & s) { + return affected.count(s.fault_code) > 0; + }), + snapshots_.end()); + } + return reclassified; } 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 a5d5f1788..82a1b5fce 100644 --- a/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp @@ -281,7 +281,7 @@ void SqliteFaultStorage::initialize_schema() { severity INTEGER NOT NULL, source_id TEXT NOT NULL ); - CREATE INDEX IF NOT EXISTS idx_near_misses_fault_code ON near_misses(fault_code, occurred_at_ns, id); + CREATE INDEX IF NOT EXISTS idx_near_misses_fault_code ON near_misses(fault_code, id); )"; if (sqlite3_exec(db_, create_near_misses_table_sql, nullptr, nullptr, &err_msg) != SQLITE_OK) { @@ -642,10 +642,19 @@ bool SqliteFaultStorage::report_fault_event(const std::string & fault_code, uint const rclcpp::Time & timestamp, const DebounceConfig & config) { std::lock_guard lock(mutex_); - // The fault row and the near-miss row have to land together. Written as separate autocommit - // statements, a failure on the second would leave the debounce counter already advanced, so the - // caller's retry would advance it a second time and the near miss it retried for would still be - // missing from the series. + // Only a FAILED report can write two rows, and only those two have to land together: written as + // separate autocommit statements, a failure on the second would leave the debounce counter + // already advanced, so the caller's retry would advance it a second time and the near miss it + // retried for would still be missing from the series. + // + // A PASSED report writes one row at most and never appends a near miss, so it keeps the plain + // autocommit path. BEGIN IMMEDIATE takes the writer lock up front, which would make a heal + // heartbeat - including one that turns out to write nothing at all - contend for that lock and + // fail with SQLITE_BUSY where before it could not. + if (event_type != EventType::EVENT_FAILED) { + return report_fault_event_locked(fault_code, event_type, severity, description, source_id, timestamp, config); + } + exec_or_throw("BEGIN IMMEDIATE"); try { const bool is_new_occurrence = @@ -1262,12 +1271,15 @@ std::optional SqliteFaultStorage::get_freeze_frame(const std::s return frame; } -void SqliteFaultStorage::set_max_near_misses_per_fault(size_t max_count) { +size_t SqliteFaultStorage::set_max_near_misses_per_fault(size_t max_count) { std::lock_guard lock(mutex_); max_near_misses_per_fault_ = max_count; - if (max_count == 0) { - return; // Unlimited + // 0 and any bound past what SQLite can hold both mean "keep everything". Binding SIZE_MAX + // straight into an int64 makes it -1, and every row then compares as beyond the bound, so the + // idiomatic spelling of "no limit" would empty the table. + if (max_count == 0 || max_count > static_cast(std::numeric_limits::max())) { + return 0; // Unlimited } // Apply the bound to what is already in the database. Without this, a database that grew under @@ -1283,6 +1295,11 @@ void SqliteFaultStorage::set_max_near_misses_per_fault(size_t max_count) { if (trim_stmt.step() != SQLITE_DONE) { throw std::runtime_error(std::string("Failed to apply near-miss bound: ") + sqlite3_errmsg(db_)); } + + // Returned rather than logged: the storage layer has no logger, and a bound applied by mistake + // deletes history that cannot be recovered, so the caller has to be able to report it. + const int changed = sqlite3_changes(db_); + return changed > 0 ? static_cast(changed) : 0; } void SqliteFaultStorage::record_near_miss_locked(const std::string & fault_code, int64_t occurred_at_ns, @@ -1302,7 +1319,8 @@ void SqliteFaultStorage::record_near_miss_locked(const std::string & fault_code, throw std::runtime_error(std::string("Failed to store near miss: ") + sqlite3_errmsg(db_)); } - if (max_near_misses_per_fault_ == 0) { + if (max_near_misses_per_fault_ == 0 || + max_near_misses_per_fault_ > static_cast(std::numeric_limits::max())) { return; // Unlimited } 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 d68f28fd0..924a03d66 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -2470,6 +2470,129 @@ TEST(FaultAuditFailClosedTest, FailClosedAbortsAndFlags) { remove_audit_files(audit_path); } +// --- InMemoryFaultStorage snapshot retention tests --- + +static void store_two_snapshots(InMemoryFaultStorage & storage, const std::string & fault_code) { + for (int i = 0; i < 2; ++i) { + ros2_medkit_fault_manager::SnapshotData snapshot; + snapshot.fault_code = fault_code; + snapshot.topic = "/test/topic" + std::to_string(i); + snapshot.message_type = "std_msgs/msg/String"; + snapshot.data = R"({"data": "value"})"; + snapshot.captured_at_ns = 1000 + i; + storage.store_snapshot(snapshot); + } +} + +TEST(InMemorySnapshotRetentionTest, RetainedOnClearWhenConfigured) { + InMemoryFaultStorage storage; + storage.set_retain_snapshots_on_clear(true); + + storage.report_fault_event("SNAPSHOT_RETAIN_TEST", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, + "fault with evidence", "/test_node", rclcpp::Time(1000), default_config()); + store_two_snapshots(storage, "SNAPSHOT_RETAIN_TEST"); + ASSERT_EQ(storage.get_snapshots("SNAPSHOT_RETAIN_TEST").size(), 2u); + + ASSERT_TRUE(storage.clear_fault("SNAPSHOT_RETAIN_TEST")); + + EXPECT_EQ(storage.get_snapshots("SNAPSHOT_RETAIN_TEST").size(), 2u); +} + +TEST(InMemorySnapshotRetentionTest, DeletedOnClearByDefault) { + InMemoryFaultStorage storage; + + storage.report_fault_event("SNAPSHOT_DROP_TEST", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, + "fault with evidence", "/test_node", rclcpp::Time(1000), default_config()); + store_two_snapshots(storage, "SNAPSHOT_DROP_TEST"); + + ASSERT_TRUE(storage.clear_fault("SNAPSHOT_DROP_TEST")); + + EXPECT_TRUE(storage.get_snapshots("SNAPSHOT_DROP_TEST").empty()); +} + +TEST(InMemorySnapshotRetentionTest, HealedReclassificationDropsSnapshotsByDefault) { + // The SQLite backend drops snapshots here; a backend that kept them would answer a snapshot + // query differently for the same sequence of calls. + InMemoryFaultStorage storage; + DebounceConfig config; + config.healing_enabled = true; + config.healing_threshold = 1; + + storage.report_fault_event("SNAPSHOT_DROP_TEST", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, + "fault with evidence", "/test_node", rclcpp::Time(1000), config); + store_two_snapshots(storage, "SNAPSHOT_DROP_TEST"); + for (int i = 1; i <= 2; ++i) { + storage.report_fault_event("SNAPSHOT_DROP_TEST", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_ERROR, "", + "/test_node", rclcpp::Time(1000 + i), config); + } + auto healed = storage.get_fault("SNAPSHOT_DROP_TEST"); + ASSERT_TRUE(healed.has_value()); + ASSERT_EQ(healed->status, Fault::STATUS_HEALED) << "test setup: the fault must be latched HEALED"; + + ASSERT_EQ(storage.reclassify_healed_as_cleared().size(), 1u); + + EXPECT_TRUE(storage.get_snapshots("SNAPSHOT_DROP_TEST").empty()); +} + +TEST(InMemorySnapshotRetentionTest, HealedReclassificationKeepsSnapshotsWhenConfigured) { + InMemoryFaultStorage storage; + storage.set_retain_snapshots_on_clear(true); + DebounceConfig config; + config.healing_enabled = true; + config.healing_threshold = 1; + + storage.report_fault_event("SNAPSHOT_RETAIN_TEST", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, + "fault with evidence", "/test_node", rclcpp::Time(1000), config); + store_two_snapshots(storage, "SNAPSHOT_RETAIN_TEST"); + for (int i = 1; i <= 2; ++i) { + storage.report_fault_event("SNAPSHOT_RETAIN_TEST", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_ERROR, "", + "/test_node", rclcpp::Time(1000 + i), config); + } + ASSERT_EQ(storage.reclassify_healed_as_cleared().size(), 1u); + + EXPECT_EQ(storage.get_snapshots("SNAPSHOT_RETAIN_TEST").size(), 2u); +} + +TEST(InMemorySnapshotRetentionTest, HealedReclassificationLeavesOtherFaultsSnapshotsAlone) { + InMemoryFaultStorage storage; + DebounceConfig healing; + healing.healing_enabled = true; + healing.healing_threshold = 1; + + storage.report_fault_event("HEALED_FAULT", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "fault", + "/test_node", rclcpp::Time(1000), healing); + store_two_snapshots(storage, "HEALED_FAULT"); + for (int i = 1; i <= 2; ++i) { + storage.report_fault_event("HEALED_FAULT", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_ERROR, "", + "/test_node", rclcpp::Time(1000 + i), healing); + } + + storage.report_fault_event("ACTIVE_FAULT", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "fault", + "/test_node", rclcpp::Time(1000), default_config()); + store_two_snapshots(storage, "ACTIVE_FAULT"); + + ASSERT_EQ(storage.reclassify_healed_as_cleared().size(), 1u); + + EXPECT_TRUE(storage.get_snapshots("HEALED_FAULT").empty()); + EXPECT_EQ(storage.get_snapshots("ACTIVE_FAULT").size(), 2u) << "reclassification touched an unrelated fault"; +} + +TEST(InMemorySnapshotRetentionTest, RetentionIsPerFaultCodeNotGlobal) { + InMemoryFaultStorage storage; + storage.set_retain_snapshots_on_clear(true); + + for (const auto & code : {"FAULT_A", "FAULT_B"}) { + storage.report_fault_event(code, ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "fault", "/test_node", + rclcpp::Time(1000), default_config()); + store_two_snapshots(storage, code); + } + + ASSERT_TRUE(storage.clear_fault("FAULT_A")); + + EXPECT_EQ(storage.get_snapshots("FAULT_A").size(), 2u); + EXPECT_EQ(storage.get_snapshots("FAULT_B").size(), 2u); +} + // --- InMemoryFaultStorage near-miss series tests --- // // The in-memory backend must keep the same near-miss contract as SQLite: appended per @@ -2625,6 +2748,23 @@ TEST(InMemoryNearMissTest, ApplyingSmallerBoundTrimsExistingSeries) { EXPECT_EQ(series[1].debounce_counter, -5); } +TEST(InMemoryNearMissTest, ApplyingBoundReportsHowManyEntriesItDropped) { + InMemoryFaultStorage storage; + DebounceConfig config = near_miss_config(); + config.confirmation_threshold = -20; + storage.set_max_near_misses_per_fault(0); + + for (int i = 0; i < 5; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(i), config); + storage.report_fault_event("MOTOR_OVERHEAT", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "temperature rising", "/powertrain/motor", near_miss_time(i), config); + } + + EXPECT_EQ(storage.set_max_near_misses_per_fault(2), 6u); + EXPECT_EQ(storage.set_max_near_misses_per_fault(2), 0u); +} + TEST(InMemoryNearMissTest, EmptyForUnknownFault) { InMemoryFaultStorage storage; EXPECT_TRUE(storage.get_near_misses("NEVER_REPORTED").empty()); 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 1ec7c5777..44f6904e1 100644 --- a/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp +++ b/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include #include @@ -2198,6 +2199,52 @@ TEST_F(SqliteFaultStorageTest, ApplyingUnlimitedBoundKeepsExistingSeries) { EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 4u); } +TEST_F(SqliteFaultStorageTest, UnlimitedBoundSpeltAsSizeMaxKeepsTheSeries) { + // SIZE_MAX is the idiomatic spelling of "no limit". Bound straight into an int64 it becomes -1, + // and every row then compares as beyond the bound, which would empty the table. + DebounceConfig config = four_strike_config(); + config.confirmation_threshold = -20; + + for (int i = 0; i < 3; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + + EXPECT_EQ(storage_->set_max_near_misses_per_fault(std::numeric_limits::max()), 0u); + EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 3u); + + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(3), config); + EXPECT_EQ(storage_->get_near_misses("PUMP_PRESSURE_LOW").size(), 4u); +} + +TEST_F(SqliteFaultStorageTest, ApplyingBoundReportsHowManyEntriesItDropped) { + DebounceConfig config = four_strike_config(); + config.confirmation_threshold = -20; + storage_->set_max_near_misses_per_fault(0); + + for (int i = 0; i < 5; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + storage_->report_fault_event("MOTOR_OVERHEAT", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "temperature rising", "/powertrain/motor", nth_report_time(i), config); + } + + // Two codes, five entries each, bound of 2: three dropped per code. + EXPECT_EQ(storage_->set_max_near_misses_per_fault(2), 6u); + // Applying the same bound again has nothing left to drop. + EXPECT_EQ(storage_->set_max_near_misses_per_fault(2), 0u); +} + +TEST_F(SqliteFaultStorageTest, PassedReportOnUnknownFaultWritesNothing) { + // A heal heartbeat for a fault that does not exist must stay a read: it writes no row and must + // not take the writer lock on the way to finding that out. + EXPECT_FALSE(storage_->report_fault_event("NEVER_REPORTED", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_WARN, + "", "/test_node", nth_report_time(0), default_config())); + EXPECT_EQ(storage_->size(), 0u); + EXPECT_TRUE(storage_->get_near_misses("NEVER_REPORTED").empty()); +} + TEST_F(SqliteFaultStorageTest, NearMissSeriesEmptyForUnknownFault) { EXPECT_TRUE(storage_->get_near_misses("NEVER_REPORTED").empty()); } From 56e69fd23aaa513c8c9cc24025412c574a981014 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 20 Aug 2026 20:48:47 +0200 Subject: [PATCH 4/9] feat(fault_manager): record the fault status each near miss left behind The HEALED latch holds the status the whole way from the healing threshold down to the confirmation threshold. Every FAILED report on the way back into a fault that does confirm therefore moves the counter without confirming, which is the definition of a near miss, and lands in the series next to approaches that receded. Nothing in the entry told the two apart, so the series could not answer how often a code approached confirmation without becoming a fault, and under a bound the ramps evicted the approaches. Each entry now records the fault status the report left behind. PREFAILED is an approach from a resting state, HEALED is a counter walking back down under the latch. It is never CONFIRMED, since that is what excludes a report from the series. Rows written before the column existed read it as empty. Also state in the interface and the docs that with per-entity overrides the recorded confirmation threshold is the reporting source's, while the counter is shared by every source of that fault code, so it is not on its own the distance to confirmation. --- docs/config/fault-manager.rst | 12 ++- src/ros2_medkit_fault_manager/README.md | 17 +++- .../fault_storage.hpp | 22 +++-- .../sqlite_fault_storage.hpp | 4 +- .../src/fault_storage.cpp | 1 + .../src/sqlite_fault_storage.cpp | 38 +++++++-- .../test/test_fault_manager.cpp | 26 ++++++ .../test/test_sqlite_storage.cpp | 85 +++++++++++++++++++ 8 files changed, 189 insertions(+), 16 deletions(-) diff --git a/docs/config/fault-manager.rst b/docs/config/fault-manager.rst index b548e7a0f..4bddb7327 100644 --- a/docs/config/fault-manager.rst +++ b/docs/config/fault-manager.rst @@ -83,8 +83,16 @@ CONFIRMED - the fault nearly happened. PASSED reports move the counter in the he (the fault receding) and are not near misses. The fault manager appends one entry per near miss to a per-fault-code series, holding the -timestamp, the counter value after the report, the confirmation threshold, the severity and the -reporting source. The series is **retained when the fault is cleared**, because acknowledging one +timestamp, the counter value after the report, the confirmation threshold, the severity, the +reporting source and the fault status the report left behind. + +The status matters when reading the series. The HEALED latch holds the status all the way from the +healing threshold down to the confirmation threshold, so reports on the way back into a fault that +does confirm are also near misses by the definition above. Entries recording ``PREFAILED`` are +approaches from a resting state; entries recording ``HEALED`` are a counter walking back down under +the latch. The recorded confirmation threshold belongs to the reporting source, while the counter +is shared by all sources of that fault code, so with per-entity thresholds it is not on its own the +distance to confirmation. The series is **retained when the fault is cleared**, because acknowledging one fault cycle must not erase how often that code approached confirmation across cycles. .. code-block:: yaml diff --git a/src/ros2_medkit_fault_manager/README.md b/src/ros2_medkit_fault_manager/README.md index bea6d7c46..8ca98523f 100644 --- a/src/ros2_medkit_fault_manager/README.md +++ b/src/ros2_medkit_fault_manager/README.md @@ -142,11 +142,24 @@ to any scoring sense used elsewhere. PASSED reports move the counter too, but in direction (the fault receding), so they are not near misses. The series is **append-only**: one entry per qualifying report, holding the timestamp, the counter -value after the report, the confirmation threshold it was measured against, the severity and the -reporting source. Every field describes that one report, so a series spanning a threshold change +value after the report, the confirmation threshold it was measured against, the severity, the +reporting source, and the fault status the report left behind. Every field describes that one report, so a series spanning a threshold change or a new reporting source stays readable. Nothing is updated in place, so "this code approached confirmation five times this hour" stays answerable. +Read `resulting_status` before counting entries. The HEALED latch holds the status the whole way +from the healing threshold down to the confirmation threshold, so every report on the way back +into a fault that **does** confirm also moves the counter without confirming, and lands in the +series. Entries with `PREFAILED` are approaches from a resting state; entries with `HEALED` are a +counter walking back down under the latch. Without the field the two cannot be told apart, and +"how often did this code approach confirmation without becoming a fault" is not answerable. +`resulting_status` is never `CONFIRMED` - that is what makes a report a near miss. It is empty for +rows written before the field existed. + +With per-entity thresholds the recorded `confirmation_threshold` is the one belonging to the +**reporting source**, while the debounce counter is shared by every source of that fault code. It +is therefore not by itself the distance to confirmation for the fault as a whole. + Entries are kept and evicted in **arrival order**, not by their timestamps. Reporters carry their own clocks, so a report can arrive carrying a timestamp behind one already stored; ordering the series by timestamp would let such a report evict itself on arrival. 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 16e2bca49..3bc60dc35 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 @@ -162,11 +162,23 @@ std::string rosbag_recording_id(const std::string & file_path); /// long-running appliance keeps the recent series rather than freezing it at boot. struct NearMissRecord { std::string fault_code; - int64_t occurred_at_ns{0}; ///< Timestamp of the report that moved the counter - int32_t debounce_counter{0}; ///< Counter value AFTER this report - int32_t confirmation_threshold{0}; ///< Counter value that would have confirmed the fault - uint8_t severity{0}; ///< Severity carried by the report - std::string source_id; ///< Reporting source + int64_t occurred_at_ns{0}; ///< Timestamp of the report that moved the counter + int32_t debounce_counter{0}; ///< Counter value AFTER this report + + /// Confirmation threshold the report was evaluated against. With per-entity overrides this is + /// the threshold of the REPORTING SOURCE, while the counter is shared by every source of the + /// code, so it is not by itself the distance to confirmation for the fault as a whole. + int32_t confirmation_threshold{0}; + + uint8_t severity{0}; ///< Severity carried by the report + std::string source_id; ///< Reporting source + + /// Fault status after the report was applied. Never CONFIRMED - that is what makes the report a + /// near miss. It separates a counter climbing from a resting state (PREFAILED) from one walking + /// back down under the HEALED latch, which is on its way to a fault that does confirm. Without + /// it the two are indistinguishable and the series cannot answer how often the code approached + /// confirmation WITHOUT becoming a fault. + std::string resulting_status; }; /// One row = one LINK: a fault claiming a recording. Several faults of a burst link to 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 2cd4909b6..3565febf4 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 @@ -148,8 +148,10 @@ class SqliteFaultStorage : public FaultStorage { /// @param config Debounce config the report was evaluated against /// @param severity Severity carried by the report /// @param source_id Reporting source + /// @param resulting_status Fault status after the report was applied void record_near_miss_locked(const std::string & fault_code, int64_t occurred_at_ns, int32_t debounce_counter, - const DebounceConfig & config, uint8_t severity, const std::string & source_id); + const DebounceConfig & config, uint8_t severity, const std::string & source_id, + const std::string & resulting_status); /// Run a plain SQL statement or throw with the SQLite error. Caller holds mutex_. void exec_or_throw(const char * sql); diff --git a/src/ros2_medkit_fault_manager/src/fault_storage.cpp b/src/ros2_medkit_fault_manager/src/fault_storage.cpp index d4e459d8f..bbb00f40c 100644 --- a/src/ros2_medkit_fault_manager/src/fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_storage.cpp @@ -500,6 +500,7 @@ void InMemoryFaultStorage::record_near_miss(const FaultState & state, const Debo record.confirmation_threshold = config.confirmation_threshold; record.severity = severity; record.source_id = source_id; + record.resulting_status = state.status; series.push_back(std::move(record)); // Evict oldest-first: a series frozen at boot says nothing about a trend. 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 82a1b5fce..6b6a37bf1 100644 --- a/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp @@ -279,7 +279,8 @@ void SqliteFaultStorage::initialize_schema() { debounce_counter INTEGER NOT NULL, confirmation_threshold INTEGER NOT NULL, severity INTEGER NOT NULL, - source_id TEXT NOT NULL + source_id TEXT NOT NULL, + resulting_status TEXT NOT NULL DEFAULT '' ); CREATE INDEX IF NOT EXISTS idx_near_misses_fault_code ON near_misses(fault_code, id); )"; @@ -290,6 +291,27 @@ void SqliteFaultStorage::initialize_schema() { throw std::runtime_error("Failed to create near_misses table: " + error); } + // Migration: rows written before resulting_status existed keep their data; the column arrives + // empty, which consumers read as "latch state not recorded". + { + bool has_resulting_status = false; + SqliteStatement info(db_, "PRAGMA table_info(near_misses)"); + while (info.step() == SQLITE_ROW) { + if (info.column_text(1) == "resulting_status") { + has_resulting_status = true; + break; + } + } + if (!has_resulting_status) { + if (sqlite3_exec(db_, "ALTER TABLE near_misses ADD COLUMN resulting_status TEXT NOT NULL DEFAULT ''", nullptr, + nullptr, &err_msg) != SQLITE_OK) { + std::string error = err_msg ? err_msg : "Unknown error"; + sqlite3_free(err_msg); + throw std::runtime_error("Failed to add resulting_status column: " + error); + } + } + } + // Create rosbag_files table. One row = one LINK (a fault claiming a recording): // several faults of a burst link to one bag, and one fault links to several bags // over time. Bytes belong to file_path, not to the row. @@ -788,7 +810,7 @@ bool SqliteFaultStorage::report_fault_event_locked(const std::string & fault_cod } if (is_near_miss(true, new_status)) { - record_near_miss_locked(fault_code, timestamp_ns, debounce_counter, config, severity, source_id); + record_near_miss_locked(fault_code, timestamp_ns, debounce_counter, config, severity, source_id, new_status); } } else { // PASSED event - increment towards healing, clamped to the thresholds. @@ -854,7 +876,7 @@ bool SqliteFaultStorage::report_fault_event_locked(const std::string & fault_cod } if (is_near_miss(true, initial_status)) { - record_near_miss_locked(fault_code, timestamp_ns, initial_counter, config, severity, source_id); + record_near_miss_locked(fault_code, timestamp_ns, initial_counter, config, severity, source_id, initial_status); } return true; // New fault created @@ -1304,16 +1326,19 @@ size_t SqliteFaultStorage::set_max_near_misses_per_fault(size_t max_count) { void SqliteFaultStorage::record_near_miss_locked(const std::string & fault_code, int64_t occurred_at_ns, int32_t debounce_counter, const DebounceConfig & config, - uint8_t severity, const std::string & source_id) { + uint8_t severity, const std::string & source_id, + const std::string & resulting_status) { SqliteStatement insert_stmt(db_, "INSERT INTO near_misses (fault_code, occurred_at_ns, debounce_counter, " - "confirmation_threshold, severity, source_id) VALUES (?, ?, ?, ?, ?, ?)"); + "confirmation_threshold, severity, source_id, resulting_status) " + "VALUES (?, ?, ?, ?, ?, ?, ?)"); insert_stmt.bind_text(1, fault_code); insert_stmt.bind_int64(2, occurred_at_ns); insert_stmt.bind_int(3, debounce_counter); insert_stmt.bind_int(4, config.confirmation_threshold); insert_stmt.bind_int(5, static_cast(severity)); insert_stmt.bind_text(6, source_id); + insert_stmt.bind_text(7, resulting_status); if (insert_stmt.step() != SQLITE_DONE) { throw std::runtime_error(std::string("Failed to store near miss: ") + sqlite3_errmsg(db_)); @@ -1351,7 +1376,7 @@ std::vector SqliteFaultStorage::get_near_misses(const std::strin SqliteStatement stmt(db_, "SELECT fault_code, occurred_at_ns, debounce_counter, confirmation_threshold, " - "severity, source_id FROM near_misses WHERE fault_code = ? " + "severity, source_id, resulting_status FROM near_misses WHERE fault_code = ? " "ORDER BY id ASC"); stmt.bind_text(1, fault_code); @@ -1363,6 +1388,7 @@ std::vector SqliteFaultStorage::get_near_misses(const std::strin record.confirmation_threshold = stmt.column_int(3); record.severity = static_cast(stmt.column_int(4)); record.source_id = stmt.column_text(5); + record.resulting_status = stmt.column_text(6); result.push_back(std::move(record)); } 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 924a03d66..18cc04a18 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -2765,6 +2765,32 @@ TEST(InMemoryNearMissTest, ApplyingBoundReportsHowManyEntriesItDropped) { EXPECT_EQ(storage.set_max_near_misses_per_fault(2), 0u); } +TEST(InMemoryNearMissTest, EntriesCarryTheResultingStatus) { + InMemoryFaultStorage storage; + DebounceConfig config = near_miss_config(); + config.healing_enabled = true; + config.healing_threshold = 2; + + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(0), config); + for (int i = 1; i <= 3; ++i) { + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_WARN, "", + "/hydraulics/pump", near_miss_time(i), config); + } + auto healed = storage.get_fault("PUMP_PRESSURE_LOW"); + ASSERT_TRUE(healed.has_value()); + ASSERT_EQ(healed->status, Fault::STATUS_HEALED) << "test setup: the fault must be latched HEALED"; + + storage.report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", near_miss_time(4), config); + + auto series = storage.get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 2u); + EXPECT_EQ(series[0].resulting_status, Fault::STATUS_PREFAILED); + EXPECT_EQ(series[1].resulting_status, Fault::STATUS_HEALED) + << "a report under the HEALED latch must not read as a fresh approach"; +} + TEST(InMemoryNearMissTest, EmptyForUnknownFault) { InMemoryFaultStorage storage; EXPECT_TRUE(storage.get_near_misses("NEVER_REPORTED").empty()); 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 44f6904e1..0d721a799 100644 --- a/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp +++ b/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp @@ -2245,6 +2245,91 @@ TEST_F(SqliteFaultStorageTest, PassedReportOnUnknownFaultWritesNothing) { EXPECT_TRUE(storage_->get_near_misses("NEVER_REPORTED").empty()); } +TEST_F(SqliteFaultStorageTest, NearMissSeparatesApproachFromRampBackIntoAFault) { + // The HEALED latch holds the status all the way down to confirmation, so every FAILED report on + // the way back into a fault that DOES confirm looks like an approach. resulting_status is what + // tells the two apart afterwards. + DebounceConfig config = four_strike_config(); + config.healing_enabled = true; + config.healing_threshold = 2; + + // An approach that recedes: counter moves, nothing is latched. + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(0), config); + for (int i = 1; i <= 3; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_WARN, "", + "/hydraulics/pump", nth_report_time(i), config); + } + auto healed = storage_->get_fault("PUMP_PRESSURE_LOW"); + ASSERT_TRUE(healed.has_value()); + ASSERT_EQ(healed->status, Fault::STATUS_HEALED) << "test setup: the fault must be latched HEALED"; + + // The ramp back down: the latch keeps reporting HEALED until the fault actually confirms. + // From the healing threshold (+2) it takes six FAILED reports to reach the confirmation + // threshold (-4). + for (int i = 4; i <= 9; ++i) { + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(i), config); + } + auto confirmed = storage_->get_fault("PUMP_PRESSURE_LOW"); + ASSERT_TRUE(confirmed.has_value()); + ASSERT_EQ(confirmed->status, Fault::STATUS_CONFIRMED) << "test setup: the ramp must end in a real fault"; + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_FALSE(series.empty()); + EXPECT_EQ(series[0].resulting_status, Fault::STATUS_PREFAILED) << "the first report was a genuine approach"; + + size_t approaches = 0; + for (const auto & entry : series) { + EXPECT_NE(entry.resulting_status, Fault::STATUS_CONFIRMED) << "a confirmed report is not a near miss"; + if (entry.resulting_status == Fault::STATUS_PREFAILED) { + ++approaches; + } + } + EXPECT_EQ(approaches, 1u) << "the ramp into a real fault must not read as an approach that receded"; +} + +TEST_F(SqliteFaultStorageTest, NearMissResultingStatusSurvivesReopen) { + const auto config = four_strike_config(); + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(0), config); + + storage_.reset(); + storage_ = std::make_unique(temp_db_path_.string()); + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 1u); + EXPECT_EQ(series[0].resulting_status, Fault::STATUS_PREFAILED); +} + +TEST_F(SqliteFaultStorageTest, NearMissResultingStatusColumnAddedToOlderTable) { + // A database written by an earlier build of this branch has the table without the column. + const auto config = four_strike_config(); + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(0), config); + storage_.reset(); + + { + sqlite3 * raw = nullptr; + ASSERT_EQ(sqlite3_open(temp_db_path_.string().c_str(), &raw), SQLITE_OK); + ASSERT_EQ(sqlite3_exec(raw, "ALTER TABLE near_misses DROP COLUMN resulting_status", nullptr, nullptr, nullptr), + SQLITE_OK); + sqlite3_close(raw); + } + + storage_ = std::make_unique(temp_db_path_.string()); + + auto series = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(series.size(), 1u) << "the migration must keep the rows it already had"; + EXPECT_TRUE(series[0].resulting_status.empty()) << "an unrecorded latch state reads as empty, not as a status"; + + storage_->report_fault_event("PUMP_PRESSURE_LOW", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, + "pressure dipping", "/hydraulics/pump", nth_report_time(1), config); + auto extended = storage_->get_near_misses("PUMP_PRESSURE_LOW"); + ASSERT_EQ(extended.size(), 2u); + EXPECT_EQ(extended[1].resulting_status, Fault::STATUS_PREFAILED); +} + TEST_F(SqliteFaultStorageTest, NearMissSeriesEmptyForUnknownFault) { EXPECT_TRUE(storage_->get_near_misses("NEVER_REPORTED").empty()); } From 67ec84a820c4ee27eed40f410fc0e979b038989c Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 20 Aug 2026 20:53:51 +0200 Subject: [PATCH 5/9] fix(fault_manager): clamp the stored debounce counter into the reporting config's band The SQLite backend brings a stored counter back into range before applying a report; the in-memory one did not. Per-entity threshold overrides mean two sources of the same fault code are evaluated against different bands, so a counter clamped to one source's ceiling can sit above another's, and clamp(clamp(x) - 1) is not clamp(x - 1) once it does. Driving a code to a wide source's healing ceiling and then reporting FAILED from a narrower source recorded counter 2 in SQLite and 3 in memory, offsetting the whole series and the report at which the fault confirms for the rest of its life. The in-memory backend now clamps the same way, and a test drives one sequence through both backends and compares what they store. --- .../src/fault_storage.cpp | 7 +++ .../test/test_fault_manager.cpp | 62 +++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/src/ros2_medkit_fault_manager/src/fault_storage.cpp b/src/ros2_medkit_fault_manager/src/fault_storage.cpp index bbb00f40c..34f9053e8 100644 --- a/src/ros2_medkit_fault_manager/src/fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_storage.cpp @@ -197,6 +197,13 @@ bool InMemoryFaultStorage::report_fault_event(const std::string & fault_code, ui return true; // Reactivation treated as new occurrence for event publishing } + // Bring a counter left outside this config's band back into range before applying the report. + // Per-entity threshold overrides mean two sources of the same fault code can be evaluated + // against different bands, so a stored value clamped to one source's ceiling can sit above + // another's. The SQLite backend clamps on read for the same reason, and the two backends have to + // agree on the counter they record and on the status it produces. + state.debounce_counter = clamp_debounce_counter(state.debounce_counter, config); + if (is_failed) { // last_occurred tracks occurrences only. A PASSED event is the fault ENDING, not // occurring; bumping it there makes a long-stale CONFIRMED fault look freshly 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 18cc04a18..bf6423ca7 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -2791,6 +2791,68 @@ TEST(InMemoryNearMissTest, EntriesCarryTheResultingStatus) { << "a report under the HEALED latch must not read as a fresh approach"; } +/// Run one identical report sequence against both backends and return their near-miss series. +/// Per-entity threshold overrides mean two reports for the SAME fault code can carry different +/// DebounceConfig values, which is the only way a stored counter ends up outside the band the next +/// report is evaluated against. +TEST(StorageBackendParityTest, MixedEntityThresholdsProduceTheSameSeries) { + std::random_device rd; + std::mt19937 gen(rd()); + std::uniform_int_distribution dist; + const auto db_path = + std::filesystem::temp_directory_path() / ("test_backend_parity_" + std::to_string(dist(gen)) + ".db"); + + DebounceConfig wide; // the band the counter is driven up in + wide.confirmation_threshold = -4; + wide.healing_threshold = 6; + wide.critical_immediate_confirm = false; + + DebounceConfig narrow; // a second source, with a lower ceiling + narrow.confirmation_threshold = -4; + narrow.healing_threshold = 3; + narrow.critical_immediate_confirm = false; + + auto drive = [&](ros2_medkit_fault_manager::FaultStorage & storage) { + storage.report_fault_event("SHARED_CODE", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, "dip", + "/wide_source", rclcpp::Time(1000), wide); + for (int i = 1; i <= 7; ++i) { + storage.report_fault_event("SHARED_CODE", ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_WARN, "", + "/wide_source", rclcpp::Time(1000 + i), wide); + } + storage.report_fault_event("SHARED_CODE", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_WARN, "dip", + "/narrow_source", rclcpp::Time(2000), narrow); + }; + + InMemoryFaultStorage memory; + drive(memory); + const auto memory_series = memory.get_near_misses("SHARED_CODE"); + const auto memory_fault = memory.get_fault("SHARED_CODE"); + + std::vector sqlite_series; + std::optional sqlite_fault; + { + ros2_medkit_fault_manager::SqliteFaultStorage sqlite(db_path.string()); + drive(sqlite); + sqlite_series = sqlite.get_near_misses("SHARED_CODE"); + sqlite_fault = sqlite.get_fault("SHARED_CODE"); + } + std::filesystem::remove(db_path); + std::filesystem::remove(db_path.string() + "-wal"); + std::filesystem::remove(db_path.string() + "-shm"); + + ASSERT_TRUE(memory_fault.has_value()); + ASSERT_TRUE(sqlite_fault.has_value()); + EXPECT_EQ(memory_fault->status, sqlite_fault->status); + + ASSERT_EQ(memory_series.size(), sqlite_series.size()); + for (size_t i = 0; i < memory_series.size(); ++i) { + EXPECT_EQ(memory_series[i].debounce_counter, sqlite_series[i].debounce_counter) + << "backends disagree on the counter recorded for entry " << i; + EXPECT_EQ(memory_series[i].resulting_status, sqlite_series[i].resulting_status) + << "backends disagree on the status recorded for entry " << i; + } +} + TEST(InMemoryNearMissTest, EmptyForUnknownFault) { InMemoryFaultStorage storage; EXPECT_TRUE(storage.get_near_misses("NEVER_REPORTED").empty()); From c7667e089aeae47497c67d7c8fefe8e523db0d01 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 20 Aug 2026 21:03:34 +0200 Subject: [PATCH 6/9] fix(fault_manager): serve the newest snapshot and keep the freeze-frame visible Two defects in how the node builds a snapshot response, both reachable once snapshots outlive the acknowledgement. get_snapshots writes one entry per topic and let the last row processed win. A topic can carry several snapshots, from re-confirmations within a cycle and now from every retained occurrence, and the backends return them in opposite orders: SQLite newest first, memory oldest first. So SQLite served the OLDEST value for a topic while reporting the newest captured_at above it, and the two backends answered the same query differently. The newest capture per topic is now tracked explicitly. get_fault served the freeze-frame only when no snapshots remained, which was the signal that acknowledgement had removed them. With retention on they never run out, so the frame - the state at the most recent confirmation - stayed hidden behind snapshots of earlier occurrences. It is served in that case too. --- .../fault_manager_node.hpp | 1 + .../src/fault_manager_node.cpp | 26 +++- .../test/test_fault_manager.cpp | 131 ++++++++++++++++++ 3 files changed, 153 insertions(+), 5 deletions(-) diff --git a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_manager_node.hpp b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_manager_node.hpp index 0f7a5cc1f..6d202e68c 100644 --- a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_manager_node.hpp +++ b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/fault_manager_node.hpp @@ -219,6 +219,7 @@ class FaultManagerNode : public rclcpp::Node { /// committed fault-state change (the fault store is a separate DB); it is not /// atomicity. bool audit_fail_closed_{false}; + std::atomic audit_dropped_writes_{0}; ///< Count of audit appends that failed (lost rows) std::atomic audit_healthy_{true}; ///< Cleared on the first failed audit append 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 c23eb9cc8..9ab128204 100644 --- a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -1076,10 +1077,14 @@ void FaultManagerNode::handle_get_fault(const std::shared_ptrenvironment_data.snapshots.push_back(snapshot); } - // Per-topic snapshots are deleted on clear_fault, but the compact freeze-frame row is - // retained. When no snapshots remain, serve the retained frame so the confirmed-state - // record stays observable after acknowledgement. - if (stored_snapshots.empty()) { + // Per-topic snapshots are deleted on clear_fault unless snapshots.retain_on_clear is set, but + // the compact freeze-frame row is retained either way. When no snapshots remain, serve the + // retained frame so the confirmed-state record stays observable after acknowledgement. + // + // With retention on, snapshots never run out, so "none left" would never fire and the frame - + // which is the state at the MOST RECENT confirmation - would stay hidden behind snapshots of + // earlier occurrences. Serve it in that case too. + if (stored_snapshots.empty() || storage_->retains_snapshots_on_clear()) { auto frame = storage_->get_freeze_frame(request->fault_code); if (frame) { ros2_medkit_msgs::msg::Snapshot snapshot; @@ -1405,9 +1410,20 @@ void FaultManagerNode::handle_get_snapshots( result["captured_at"] = static_cast(latest_captured_at) / 1e9; } - // Build topics object + // Build topics object. One topic can carry several snapshots - re-confirmations within a cycle, + // and every earlier occurrence once snapshots.retain_on_clear keeps them - and only one of them + // fits the topic's key. Keep the newest, tracked explicitly: the backends return these in + // opposite orders, so relying on the last write to win serves the oldest value from SQLite and + // the newest from memory, both under the latest captured_at. nlohmann::json topics_json = nlohmann::json::object(); + std::map newest_per_topic; for (const auto & snapshot : snapshots) { + auto seen = newest_per_topic.find(snapshot.topic); + if (seen != newest_per_topic.end() && seen->second >= snapshot.captured_at_ns) { + continue; + } + newest_per_topic[snapshot.topic] = snapshot.captured_at_ns; + nlohmann::json topic_entry; topic_entry["message_type"] = snapshot.message_type; 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 bf6423ca7..92b0abaaf 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -38,6 +38,7 @@ #include "ros2_medkit_msgs/msg/snapshot.hpp" #include "ros2_medkit_msgs/srv/clear_fault.hpp" #include "ros2_medkit_msgs/srv/get_fault.hpp" +#include "ros2_medkit_msgs/srv/get_snapshots.hpp" #include "ros2_medkit_msgs/srv/list_faults_for_entity.hpp" #include "ros2_medkit_msgs/srv/report_fault.hpp" @@ -2858,6 +2859,136 @@ TEST(InMemoryNearMissTest, EmptyForUnknownFault) { EXPECT_TRUE(storage.get_near_misses("NEVER_REPORTED").empty()); } +// --- Snapshot read path with retention enabled --- +// +// Driven through the real services, because both defects are in how the node builds the response, +// not in what storage returns. + +class SnapshotReadPathTest : public ::testing::Test { + protected: + void SetUp() override { + std::random_device rd; + std::mt19937 gen(rd()); + std::uniform_int_distribution dist; + const auto suffix = std::to_string(dist(gen)); + db_path_ = std::filesystem::temp_directory_path() / ("test_snapshot_read_" + suffix + ".db"); + const std::string ns = "/test_snapshot_read_" + suffix; + + rclcpp::NodeOptions fm_options; + fm_options.parameter_overrides({ + {"storage_type", "sqlite"}, + {"database_path", db_path_.string()}, + {"snapshots.retain_on_clear", true}, + {"snapshots.enabled", false}, + }); + fm_options.arguments({"--ros-args", "-r", "__ns:=" + ns}); + fault_manager_ = std::make_shared(fm_options); + + rclcpp::NodeOptions test_options; + test_options.arguments({"--ros-args", "-r", "__ns:=" + ns}); + test_node_ = std::make_shared("test_snapshot_reader", test_options); + + get_snapshots_client_ = + test_node_->create_client(ns + "/fault_manager/get_snapshots"); + get_fault_client_ = test_node_->create_client(ns + "/fault_manager/get_fault"); + ASSERT_TRUE(get_snapshots_client_->wait_for_service(std::chrono::seconds(5))); + ASSERT_TRUE(get_fault_client_->wait_for_service(std::chrono::seconds(5))); + } + + void TearDown() override { + get_snapshots_client_.reset(); + get_fault_client_.reset(); + test_node_.reset(); + fault_manager_.reset(); + std::filesystem::remove(db_path_); + std::filesystem::remove(db_path_.string() + "-wal"); + std::filesystem::remove(db_path_.string() + "-shm"); + } + + /// Spin both nodes until @p future resolves, or fail. + template + bool spin_until_ready(FutureT & future, std::chrono::milliseconds timeout = std::chrono::milliseconds(5000)) { + const auto start = std::chrono::steady_clock::now(); + while (std::chrono::steady_clock::now() - start < timeout) { + rclcpp::spin_some(fault_manager_); + rclcpp::spin_some(test_node_); + if (future.wait_for(std::chrono::milliseconds(0)) == std::future_status::ready) { + return true; + } + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + } + return future.wait_for(std::chrono::milliseconds(0)) == std::future_status::ready; + } + + void store_snapshot(const std::string & fault_code, const std::string & topic, const std::string & data, + int64_t captured_at_ns) { + ros2_medkit_fault_manager::SnapshotData snapshot; + snapshot.fault_code = fault_code; + snapshot.topic = topic; + snapshot.message_type = "std_msgs/msg/String"; + snapshot.data = data; + snapshot.captured_at_ns = captured_at_ns; + fault_manager_->get_storage_for_test().store_snapshot(snapshot); + } + + std::filesystem::path db_path_; + std::shared_ptr fault_manager_; + std::shared_ptr test_node_; + rclcpp::Client::SharedPtr get_snapshots_client_; + rclcpp::Client::SharedPtr get_fault_client_; +}; + +TEST_F(SnapshotReadPathTest, NewestSnapshotOfATopicWins) { + auto & storage = fault_manager_->get_storage_for_test(); + storage.report_fault_event("PLC_PRESSURE_HIGH", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, + "pressure high", "/plc", rclcpp::Time(1000), DebounceConfig{}); + store_snapshot("PLC_PRESSURE_HIGH", "/plc/pressure", R"({"data":"old"})", 1000); + store_snapshot("PLC_PRESSURE_HIGH", "/plc/pressure", R"({"data":"new"})", 5000); + + auto request = std::make_shared(); + request->fault_code = "PLC_PRESSURE_HIGH"; + auto future = get_snapshots_client_->async_send_request(request); + ASSERT_TRUE(spin_until_ready(future)); + + auto response = future.get(); + ASSERT_TRUE(response->success); + const auto payload = nlohmann::json::parse(response->data); + ASSERT_TRUE(payload.contains("topics")); + ASSERT_TRUE(payload["topics"].contains("/plc/pressure")); + EXPECT_EQ(payload["topics"]["/plc/pressure"]["data"]["data"], "new") + << "captured_at reports the newest snapshot, so the data under it must be the newest too"; +} + +TEST_F(SnapshotReadPathTest, FreezeFrameStaysVisibleBehindRetainedSnapshots) { + auto & storage = fault_manager_->get_storage_for_test(); + storage.report_fault_event("PLC_PRESSURE_HIGH", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, + "pressure high", "/plc", rclcpp::Time(1000), DebounceConfig{}); + store_snapshot("PLC_PRESSURE_HIGH", "/plc/pressure", R"({"data":"from an earlier occurrence"})", 1000); + + ros2_medkit_fault_manager::FreezeFrameData frame; + frame.fault_code = "PLC_PRESSURE_HIGH"; + frame.data = R"({"/plc/pressure":{"data":"latest confirmation"}})"; + frame.captured_at_ns = 9000; + storage.store_freeze_frame(frame); + + auto request = std::make_shared(); + request->fault_code = "PLC_PRESSURE_HIGH"; + auto future = get_fault_client_->async_send_request(request); + ASSERT_TRUE(spin_until_ready(future)); + + auto response = future.get(); + ASSERT_TRUE(response->success); + + bool frame_served = false; + for (const auto & snapshot : response->environment_data.snapshots) { + if (snapshot.name == "freeze_frame") { + frame_served = true; + EXPECT_EQ(snapshot.data, frame.data); + } + } + EXPECT_TRUE(frame_served) << "retained snapshots must not hide the most recent freeze-frame"; +} + int main(int argc, char ** argv) { rclcpp::init(argc, argv); ::testing::InitGoogleTest(&argc, argv); From cd15ccdc01e0b0567c8d6e59c5459e2742469a83 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Thu, 20 Aug 2026 21:05:14 +0200 Subject: [PATCH 7/9] docs(fault_manager): describe the snapshot read path under retention and the scope of the bound --- src/ros2_medkit_fault_manager/README.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/ros2_medkit_fault_manager/README.md b/src/ros2_medkit_fault_manager/README.md index 8ca98523f..135c10647 100644 --- a/src/ros2_medkit_fault_manager/README.md +++ b/src/ros2_medkit_fault_manager/README.md @@ -170,6 +170,15 @@ for the startup reclassification of HEALED faults. Per-topic snapshots are dropp default, because they belong to the one confirmed occurrence rather than to the series; set `snapshots.retain_on_clear` to keep them as well. +With `snapshots.retain_on_clear` on, `~/get_fault` keeps serving the freeze-frame alongside the +retained snapshots, because it records the most recent confirmation while the snapshots may belong +to earlier ones. `~/get_snapshots` returns one entry per topic and serves the newest capture of +that topic, whichever storage backend is in use. + +The bound is **per fault code, not per database**. Fault codes are unbounded in cardinality, so a +reporter emitting a stream of distinct codes still grows the table; the bound caps what any single +code costs, not the total. + Retention is **bounded per fault code** by `near_miss.max_per_fault` (default 200), evicting the **oldest** entries first. That is deliberately the opposite of the snapshot limit's keep-earliest rule: a series frozen at boot says nothing about whether the rate is changing. Set it to 0 for From 3f9a8340893b954a0e2669cc9d3987ef2043fc4a Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Fri, 21 Aug 2026 08:34:36 +0200 Subject: [PATCH 8/9] fix(fault_manager): apply snapshot retention to the startup reclassification clear_fault is not the only place that takes a fault's readings. When healing is disabled, startup reclassifies HEALED faults as CLEARED and deletes their snapshots along the way, and that path ignored snapshots.retain_on_clear. The setting therefore held only until the next restart, which then deleted exactly what it was set to keep, and the two storage backends disagreed: the in-memory one kept the snapshots while SQLite dropped them. --- .../src/sqlite_fault_storage.cpp | 15 +++-- .../test/test_sqlite_storage.cpp | 57 +++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) 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 6b6a37bf1..45af5cf54 100644 --- a/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp @@ -1033,11 +1033,16 @@ std::vector SqliteFaultStorage::reclassify_healed_as_cleared() { } // Drop snapshots for the affected faults so a reclassified row matches CLEARED semantics. - SqliteStatement del(db_, - "DELETE FROM snapshots WHERE fault_code IN (SELECT fault_code FROM faults WHERE status = ?)"); - del.bind_text(1, ros2_medkit_msgs::msg::Fault::STATUS_HEALED); - if (del.step() != SQLITE_DONE) { - throw std::runtime_error(std::string("Failed to delete snapshots: ") + sqlite3_errmsg(db_)); + // clear_fault is not the only place that takes a fault's readings, so retain_snapshots_on_clear_ + // has to reach here too: otherwise the setting holds until the next restart and then the + // reclassification deletes exactly what it was set to keep. + if (!retain_snapshots_on_clear_) { + SqliteStatement del(db_, + "DELETE FROM snapshots WHERE fault_code IN (SELECT fault_code FROM faults WHERE status = ?)"); + del.bind_text(1, ros2_medkit_msgs::msg::Fault::STATUS_HEALED); + if (del.step() != SQLITE_DONE) { + throw std::runtime_error(std::string("Failed to delete snapshots: ") + sqlite3_errmsg(db_)); + } } SqliteStatement stmt(db_, "UPDATE faults SET status = ? WHERE status = ?"); 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 0d721a799..391ddd908 100644 --- a/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp +++ b/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp @@ -2334,6 +2334,63 @@ TEST_F(SqliteFaultStorageTest, NearMissSeriesEmptyForUnknownFault) { EXPECT_TRUE(storage_->get_near_misses("NEVER_REPORTED").empty()); } +// --- Snapshot retention through the startup reclassification --- +// +// clear_fault is not the only place that drops a fault's snapshots: reclassifying a HEALED fault +// as CLEARED at startup does it too, and snapshots.retain_on_clear has to reach both. + +/// Store @p count snapshots for @p fault_code, one per topic. +static void store_snapshots_for(ros2_medkit_fault_manager::FaultStorage & storage, const std::string & fault_code, + int count) { + for (int i = 0; i < count; ++i) { + ros2_medkit_fault_manager::SnapshotData snapshot; + snapshot.fault_code = fault_code; + snapshot.topic = "/test/topic" + std::to_string(i); + snapshot.message_type = "std_msgs/msg/String"; + snapshot.data = R"({"data": "value"})"; + snapshot.captured_at_ns = 1000 + i; + storage.store_snapshot(snapshot); + } +} + +/// Drive @p fault_code to a latched HEALED state. +static void drive_to_healed(ros2_medkit_fault_manager::FaultStorage & storage, const std::string & fault_code) { + DebounceConfig config; + config.healing_enabled = true; + config.healing_threshold = 1; + + storage.report_fault_event(fault_code, ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "fault", + "/test_node", nth_report_time(0), config); + store_snapshots_for(storage, fault_code, 2); + // Two PASSED reports: the first only lifts the counter to 0, which the CONFIRMED latch holds. + for (int i = 1; i <= 2; ++i) { + storage.report_fault_event(fault_code, ReportFault::Request::EVENT_PASSED, Fault::SEVERITY_ERROR, "", "/test_node", + nth_report_time(i), config); + } +} + +TEST_F(SqliteFaultStorageTest, SnapshotsRetainedThroughHealedReclassification) { + storage_->set_retain_snapshots_on_clear(true); + drive_to_healed(*storage_, "SNAPSHOT_RETAIN_TEST"); + + auto healed = storage_->get_fault("SNAPSHOT_RETAIN_TEST"); + ASSERT_TRUE(healed.has_value()); + ASSERT_EQ(healed->status, Fault::STATUS_HEALED) << "test setup: the fault must be latched HEALED"; + ASSERT_EQ(storage_->get_snapshots("SNAPSHOT_RETAIN_TEST").size(), 2u); + + ASSERT_EQ(storage_->reclassify_healed_as_cleared().size(), 1u); + + EXPECT_EQ(storage_->get_snapshots("SNAPSHOT_RETAIN_TEST").size(), 2u) + << "the restart reclassification deleted snapshots the configuration asked to keep"; +} + +TEST_F(SqliteFaultStorageTest, SnapshotsDroppedByHealedReclassificationByDefault) { + drive_to_healed(*storage_, "SNAPSHOT_DROP_TEST"); + ASSERT_EQ(storage_->reclassify_healed_as_cleared().size(), 1u); + + EXPECT_TRUE(storage_->get_snapshots("SNAPSHOT_DROP_TEST").empty()); +} + int main(int argc, char ** argv) { rclcpp::init(argc, argv); ::testing::InitGoogleTest(&argc, argv); From 3687c341546d20a800fe59b93242348cb017fd78 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sat, 22 Aug 2026 15:58:43 +0200 Subject: [PATCH 9/9] docs(fault_manager): correct the near-miss eviction rationale The near-miss bound was described as the opposite of snapshots.max_per_fault, "which keeps the earliest". That stopped being true when the snapshot cap moved to trimming whole capture sets oldest-first: both caps now keep the newest, as does the rosbag cap, and this file already said so about the rosbag cap a few hundred lines further down. The eviction rule itself is unchanged and was already right. Only the reasoning given for it was stale, and read backwards for anyone sizing the snapshot cap. --- docs/config/fault-manager.rst | 6 +++--- src/ros2_medkit_fault_manager/README.md | 7 ++++--- src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp | 6 +++--- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/docs/config/fault-manager.rst b/docs/config/fault-manager.rst index 4bddb7327..6c9e6cdb3 100644 --- a/docs/config/fault-manager.rst +++ b/docs/config/fault-manager.rst @@ -112,9 +112,9 @@ fault cycle must not erase how often that code approached confirmation across cy * - ``near_miss.max_per_fault`` - ``200`` - Near-miss entries retained per fault code. When the bound is reached the **oldest** - entries are evicted - the opposite of ``snapshots.max_per_fault``, which keeps the - earliest, because a series frozen at boot says nothing about whether the rate of near - misses is changing. Set to 0 for unlimited, accepting growth with the reporting rate. + entries are evicted, the same direction as ``snapshots.max_per_fault`` and the rosbag cap: + a series frozen at boot says nothing about whether the rate of near misses is changing. + Set to 0 for unlimited, accepting growth with the reporting rate. .. note:: diff --git a/src/ros2_medkit_fault_manager/README.md b/src/ros2_medkit_fault_manager/README.md index 135c10647..98ac04883 100644 --- a/src/ros2_medkit_fault_manager/README.md +++ b/src/ros2_medkit_fault_manager/README.md @@ -180,9 +180,10 @@ reporter emitting a stream of distinct codes still grows the table; the bound ca code costs, not the total. Retention is **bounded per fault code** by `near_miss.max_per_fault` (default 200), evicting the -**oldest** entries first. That is deliberately the opposite of the snapshot limit's keep-earliest -rule: a series frozen at boot says nothing about whether the rate is changing. Set it to 0 for -unlimited, accepting that the database then grows with the reporting rate. +**oldest** entries first. That is the same direction as `snapshots.max_per_fault` and the rosbag +cap, and for the same reason: a series frozen at boot says nothing about whether the rate is +changing, and the evidence a technician wants is the evidence from the fault happening now. Set it +to 0 for unlimited, accepting that the database then grows with the reporting rate. The series lives in the `near_misses` table of the fault database and is read through the storage API (`FaultStorage::get_near_misses`). A database written by an earlier build gains the table on 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 45af5cf54..f1a3953ed 100644 --- a/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp @@ -1354,9 +1354,9 @@ void SqliteFaultStorage::record_near_miss_locked(const std::string & fault_code, return; // Unlimited } - // Evict oldest-first, keeping the newest max_near_misses_per_fault_ rows. Newest-first is the - // deliberate opposite of the snapshot limit's keep-earliest rule: a series frozen at boot - // answers nothing about whether the rate of near misses is changing. + // Evict oldest-first, keeping the newest max_near_misses_per_fault_ rows - the same direction as + // the snapshot and rosbag caps. A series frozen at boot answers nothing about whether the rate + // of near misses is changing. // // "Oldest" means earliest ARRIVAL (id), not earliest occurred_at_ns. Reporters carry their own // clocks, so a report can arrive with a timestamp behind one already stored; ordering eviction