diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 56c37ca1a..c12066097 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -1077,7 +1077,7 @@ Query and manage faults. { "type": "rosbag", "name": "fault_recording", - "bulk_data_uri": "/apps/motor_controller/bulk-data/rosbags/550e8400-e29b-41d4-a716-446655440000", + "bulk_data_uri": "/apps/motor_controller/bulk-data/rosbags/fault_MOTOR_OVERHEAT_1738664999000", "size_bytes": 1234567, "duration_sec": 6.0, "format": "mcap" @@ -1371,7 +1371,7 @@ Download a specific bulk-data file. .. code-block:: bash - curl -O -J http://localhost:8080/api/v1/apps/motor_controller/bulk-data/rosbags/550e8400-e29b-41d4-a716-446655440000 + curl -O -J http://localhost:8080/api/v1/apps/motor_controller/bulk-data/rosbags/fault_MOTOR_OVERHEAT_1738664999000 **Response Codes:** @@ -2814,7 +2814,8 @@ Other extensions beyond SOVD: optional ``x-medkit`` SOVD payload-extension object with ``entity_type`` and ``entity_id`` fields when the gateway can resolve the fault's first reporting source back to an entity, so consumers can hit ``/{entity_type}/{entity_id}/bulk-data/rosbags/{fault_code}`` directly - without enumerating entities. Resolution is snapshotted at event arrival; the entire + without enumerating entities - that address serves the fault's newest recording. To reach an + older one, list ``/bulk-data/rosbags`` and use the descriptor ``id``. Resolution is snapshotted at event arrival; the entire ``x-medkit`` object is omitted when no entity can be resolved. - ``/health`` - Health check with discovery pipeline diagnostics - ``/version-info`` - Gateway version information diff --git a/docs/config/fault-manager.rst b/docs/config/fault-manager.rst index 3b425418a..3100303da 100644 --- a/docs/config/fault-manager.rst +++ b/docs/config/fault-manager.rst @@ -250,6 +250,7 @@ Capture continuous rosbag recordings around fault events. max_buffer_mb: 256 # Ring-buffer RAM cap max_bag_size_mb: 50 # Max size per bag file max_total_storage_mb: 500 # Max total storage + max_bags_per_fault: 1 # Recordings kept per fault code auto_cleanup: true # Auto-delete old bags .. list-table:: @@ -324,10 +325,33 @@ Capture continuous rosbag recordings around fault events. - Maximum total storage for all rosbags (MB). A recording shared by a burst of faults counts once towards the total, and eviction removes a whole burst's bag at a time (oldest first). + * - ``rosbag.max_bags_per_fault`` + - ``1`` + - How many recordings one fault code keeps. Past the cap the oldest is + unlinked, so the default reproduces the historical behaviour exactly: a + new recording replaces the previous one. ``0`` means unlimited, bounded + only by ``max_total_storage_mb``. ``3`` is a reasonable value for a fault + that flaps - see the note below before raising it. * - ``rosbag.auto_cleanup`` - ``true`` - - Delete a fault's bag when the fault is cleared. A recording shared by a - burst survives until the last fault referencing it clears. + - Delete a fault's bags when the fault is cleared. A recording shared by a + burst survives until the last fault referencing it clears. Leave this + ``false`` when raising ``max_bags_per_fault``, or acknowledging a fault + discards the history that was just kept. + +.. note:: + + ``max_bags_per_fault`` is a **fairness** knob, not a depth knob. + ``max_total_storage_mb`` is the real disk bound and eviction across it is + global and oldest-first, so a fault that flaps often enough will consume the + budget and push out every other fault's black box. Raise the per-fault cap + when you need the history of a specific intermittent fault; raise the total + budget with it if other faults still need theirs. + + The cap keeps the newest recordings and evicts the oldest. It deliberately + does not match ``snapshots.max_per_fault``, which rejects new snapshots once + full: refusing a new recording would mean a technician standing next to a + machine faulting right now downloads a bag from three days ago. .. _rosbag-recording-lifecycle: diff --git a/docs/tutorials/snapshots.rst b/docs/tutorials/snapshots.rst index 6d8dd5134..e5bed61c8 100644 --- a/docs/tutorials/snapshots.rst +++ b/docs/tutorials/snapshots.rst @@ -266,7 +266,7 @@ Snapshots are included inline in the fault response as ``environment_data``: { "type": "rosbag", "name": "fault_recording", - "bulk_data_uri": "/apps/motor_controller/bulk-data/rosbags/550e8400-e29b-41d4-a716-446655440000", + "bulk_data_uri": "/apps/motor_controller/bulk-data/rosbags/fault_MOTOR_OVERHEAT_1738664999000", "size_bytes": 1234567, "duration_sec": 6.0, "format": "mcap" @@ -286,7 +286,10 @@ Snapshots are included inline in the fault response as ``environment_data``: gateway start instead, marked ``x-medkit.capture_origin: startup``; a plugin entity that reports its link down contributes its last known values, marked ``connected: false`` in ``x-medkit`` -- ``rosbag``: Recording file available via bulk-data endpoint (binary format) +- ``rosbag``: Recording file available via bulk-data endpoint (binary format). + One entry per recording the fault kept, newest first, each addressed by its own + ``bulk_data_uri``. With the default ``max_bags_per_fault`` of ``1`` there is at + most one. **Get snapshots from fault response using jq:** @@ -700,6 +703,11 @@ Rosbag files are downloaded via SOVD bulk-data endpoints. curl http://localhost:8080/api/v1/apps/motor_controller/bulk-data/rosbags +One item per **recording**, not per fault. A burst of correlated faults shares a +single recording and appears once, with every fault it covers listed in +``x-medkit.fault_codes``. A fault that confirmed several times contributes one +item per recording it kept (see ``max_bags_per_fault`` below). + **Response:** .. code-block:: json @@ -707,13 +715,14 @@ Rosbag files are downloaded via SOVD bulk-data endpoints. { "items": [ { - "id": "550e8400-e29b-41d4-a716-446655440000", - "name": "MOTOR_OVERHEAT recording", + "id": "fault_MOTOR_OVERHEAT_1738664999000", + "name": "fault_MOTOR_OVERHEAT_1738664999000 recording 2026-02-04T10:30:00.000Z", "mimetype": "application/x-mcap", "size": 1234567, "creation_date": "2026-02-04T10:30:00.000Z", "x-medkit": { - "fault_code": "MOTOR_OVERHEAT", + "fault_codes": ["MOTOR_OVERHEAT"], + "recording_id": "fault_MOTOR_OVERHEAT_1738664999000", "duration_sec": 6.0, "format": "mcap" } @@ -723,12 +732,15 @@ Rosbag files are downloaded via SOVD bulk-data endpoints. **2. Download a specific rosbag:** -Use the ``bulk_data_uri`` from the fault response, or construct from listing: +Use the ``bulk_data_uri`` from the fault response, or the descriptor ``id`` from +the listing. A URL carrying a bare fault code instead of a recording id still +resolves and serves that fault's newest recording, so addresses built before +recordings had their own identity keep working. .. code-block:: bash # Using bulk_data_uri from fault response - curl -O -J http://localhost:8080/api/v1/apps/motor_controller/bulk-data/rosbags/550e8400-e29b-41d4-a716-446655440000 + curl -O -J http://localhost:8080/api/v1/apps/motor_controller/bulk-data/rosbags/fault_MOTOR_OVERHEAT_1738664999000 The ``-J`` flag uses the server-provided filename from ``Content-Disposition`` header. diff --git a/src/ros2_medkit_fault_manager/CHANGELOG.rst b/src/ros2_medkit_fault_manager/CHANGELOG.rst index 1e884c910..9332f6e62 100644 --- a/src/ros2_medkit_fault_manager/CHANGELOG.rst +++ b/src/ros2_medkit_fault_manager/CHANGELOG.rst @@ -4,6 +4,7 @@ Changelog for package ros2_medkit_fault_manager Forthcoming ----------- +* Rosbag black-box recordings are no longer limited to one per fault code. A fault that re-confirms keeps a bounded history of recordings instead of overwriting the previous one, controlled by the new ``snapshots.rosbag.max_bags_per_fault`` (default ``1``, which reproduces the previous behaviour exactly; ``0`` = unlimited). Retention is keep-newest and the bag is unlinked only when no fault still references it, so a burst that shares one recording behaves as before. Internally the ``rosbag_files`` grain changed from "one row per fault" to "one row per (fault, recording) link": ``recording_id`` is now a stored, indexed column, and the legacy column-level ``UNIQUE(fault_code)`` is replaced by a ``UNIQUE INDEX`` on ``(fault_code, file_path)`` through an automatic, idempotent table rebuild on first open. Four latent defects are fixed on the way: quota eviction deleted by fault code rather than by recording, ``get_rosbag_file`` had no ``ORDER BY`` and would have served an arbitrary recording, the stale-row self-heals deleted a fault's entire history because one bag had vanished from disk, and both ``delete_rosbag_file`` / ``delete_rosbag_files`` read only the first ``file_path`` of a fault, so deleting a fault with several recordings removed every row but left all but one bag on disk - unreachable and still charged against the quota (`#620 `_) * Optional append-only, hash-chained audit log of fault state transitions: each transition appends one immutable row (``record_hash = sha256(prev_hash + canonical(event))`` via OpenSSL EVP SHA-256) with a persisted chain head, a ``verify`` routine, a read API, and retention that seals a segment anchor before pruning. Time-based (PREFAILED->CONFIRMED) auto-confirmations are also audited. ``verify`` reads the chain head directly from the database, so deleting the newest row together with the head row is reported as tampering instead of silently recovering. ``BEFORE UPDATE`` / ``BEFORE DELETE`` triggers reject out-of-band edits as defense-in-depth. The chain is unkeyed and stored in a single writable file, so ``verify`` detects edits/deletions that did not recompute the chain (casual or accidental tampering); it is not a defence against an attacker who can rewrite the whole file. Off by default (`#483 `_) 0.6.0 (2026-06-22) diff --git a/src/ros2_medkit_fault_manager/CMakeLists.txt b/src/ros2_medkit_fault_manager/CMakeLists.txt index 58eb3cc63..0225e6721 100644 --- a/src/ros2_medkit_fault_manager/CMakeLists.txt +++ b/src/ros2_medkit_fault_manager/CMakeLists.txt @@ -136,6 +136,11 @@ if(BUILD_TESTING) target_link_libraries(test_sqlite_storage fault_manager_lib) medkit_target_dependencies(test_sqlite_storage rclcpp ros2_medkit_msgs) + # Rosbag retention parity: every assertion runs against both storage backends. + medkit_add_gtest(test_rosbag_storage_parity test/test_rosbag_storage_parity.cpp) + target_link_libraries(test_rosbag_storage_parity fault_manager_lib) + medkit_target_dependencies(test_rosbag_storage_parity rclcpp ros2_medkit_msgs) + # Fault audit log tests (hash chain, verify, rotation, reopen) medkit_add_gtest(test_fault_audit_log test/test_fault_audit_log.cpp) target_link_libraries(test_fault_audit_log fault_manager_lib) @@ -190,6 +195,12 @@ if(BUILD_TESTING) medkit_add_launch_test(test_rosbag_entity_scope test/test_rosbag_entity_scope.test.py TIMEOUT 120 LABELS "integration") + # The only suite running above max_bags_per_fault=1: drives confirm / clear / + # confirm on one code and asserts both recordings survive and stay separately + # addressable. Six occurrences plus their post-roll windows, hence the timeout. + medkit_add_launch_test(test_rosbag_history test/test_rosbag_history.test.py TIMEOUT 240 + LABELS "integration") + # Parametrized over both storage formats (sqlite3 + mcap), so the launch # runs twice inside one ctest invocation. medkit_add_launch_test(test_rosbag_boundary test/test_rosbag_boundary.test.py TIMEOUT 240 diff --git a/src/ros2_medkit_fault_manager/config/snapshots.yaml b/src/ros2_medkit_fault_manager/config/snapshots.yaml index fa89b2f15..580695e5b 100644 --- a/src/ros2_medkit_fault_manager/config/snapshots.yaml +++ b/src/ros2_medkit_fault_manager/config/snapshots.yaml @@ -79,6 +79,13 @@ default_topics: # `default_topics` from this YAML file. Rosbag settings must be configured # via ROS 2 parameters (--ros-args -p snapshots.rosbag.*) or launch files. +# Keep a fault's value snapshots when it is acknowledged (default: false) +# Off is the historical behaviour: clearing a fault deletes them. Turn it on +# together with rosbag.max_bags_per_fault, or acknowledging leaves the fault +# holding recordings whose matching readings are gone - evidence that no longer +# lines up. Growth stays bounded by max_per_fault either way. +retain_on_clear: false + rosbag: # Enable/disable rosbag capture (default: false) # When disabled, only JSON snapshots are captured @@ -145,7 +152,8 @@ rosbag: # Storage path for bag files (default: "" = system temp directory) # Empty string uses /tmp/rosbag_snapshots/ - # Bag files are named: {fault_code}_{timestamp}/ + # Bag files are named: fault_{fault_code}_{timestamp}/ and that directory name is + # the recording's public id - the last segment of its bulk-data URL. storage_path: "" # Maximum size per bag file in MB (default: 50) @@ -153,9 +161,23 @@ rosbag: max_bag_size_mb: 50 # Maximum total storage for all bag files in MB (default: 500) - # Oldest bags are deleted when this limit is exceeded + # Oldest bags are deleted when this limit is exceeded. This is the real disk bound; + # max_bags_per_fault below only decides how the budget is shared out. max_total_storage_mb: 500 + # Recordings kept per fault code (default: 1, 0 = unlimited) + # A fault that keeps re-confirming leaves a trail of black boxes instead of only + # the latest one. Past the cap the fault's OLDEST recording is dropped, and the bag + # is deleted once no fault still references it (a burst shares one recording). + # + # 1 is the historical behaviour: each re-confirmation replaces the previous bag. + # 3 is a good starting point for an intermittent fault you are chasing. + # + # Think of this as fairness rather than depth: the disk is bounded by + # max_total_storage_mb either way, and a high value lets one flapping fault consume + # the budget and evict every other fault's recording. + max_bags_per_fault: 1 + # Maximum in-memory ring buffer size in MB (default: 256) # Oldest buffered messages are dropped once the buffer exceeds this, so a broad # subscribe set on a busy robot cannot grow memory without bound. 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 f75e85a0b..d027d1e9d 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 @@ -103,12 +103,18 @@ struct FaultState { using EventType = ros2_medkit_msgs::srv::ReportFault::Request; /// Snapshot data captured when a fault is confirmed +/// One captured topic value. A confirmation writes one of these PER TOPIC, and +/// all of them share a capture_id: they are one reading of the machine taken at +/// one moment and only mean anything together. struct SnapshotData { std::string fault_code; std::string topic; std::string message_type; std::string data; ///< JSON-encoded message data int64_t captured_at_ns{0}; + /// Groups the rows of one capture. Monotonic within a fault manager process; + /// 0 on rows written before the field existed, which read as one legacy set. + int64_t capture_id{0}; }; /// Compact freeze-frame captured when a fault confirms: a single JSON object mapping @@ -123,9 +129,23 @@ struct FreezeFrameData { int64_t captured_at_ns{0}; }; -/// Rosbag file metadata for time-window recording +/// Derive a recording's public identity from its bag path: the directory basename, +/// `fault__`. Faults of one burst share a recording and therefore share +/// this id. +/// +/// Safe as a URL path segment because fault codes are validated to +/// `[A-Za-z0-9_.-]` with no `..` before a bag is ever named after one, which makes +/// that validation load-bearing for a second reason. The gateway reads the same value +/// 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 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 +/// only when its last row goes. struct RosbagFileInfo { std::string fault_code; + std::string recording_id; ///< Basename of file_path; shared by every row of a burst std::string file_path; std::string format; ///< "sqlite3" or "mcap" double duration_sec{0.0}; ///< Total duration of recorded data @@ -191,6 +211,44 @@ class FaultStorage { virtual void set_max_snapshots_per_fault(size_t /*max_count*/) { } + /// Cap on RECORDINGS retained per fault code (0 = unlimited). + /// + /// Enforced inside store_rosbag_file(s), atomically with the insert: past the cap + /// the fault's OLDEST recordings lose their row, and a bag whose last referencing + /// row goes is unlinked once the store is durable. + /// + /// Keep-newest, deliberately the opposite of set_max_snapshots_per_fault's + /// reject-new. Snapshots guard against one capture writing a row per configured + /// topic and flooding the table, where keeping the earliest is right. A bag is + /// evidence about a machine you are about to inspect, so the recent one wins; and + /// keep-newest at 1 is exactly the pre-#620 behaviour, which is what makes the + /// default a no-op. + virtual void set_max_rosbags_per_fault(size_t /*max_count*/) { + } + + /// Whether acknowledging a fault keeps the value snapshots it captured. + /// + /// Off by default, which is the historical behaviour: clear_fault deletes them. + /// Turn it on together with a rosbag history, or acknowledging leaves the fault + /// holding recordings whose matching readings are gone - evidence that no longer + /// lines up. Growth stays bounded by the per-fault cap either way, so clearing + /// was never the only thing holding the table down. + virtual void set_retain_snapshots_on_clear(bool /*retain*/) { + } + + /// Store one capture as a unit. + /// + /// The per-fault cap applies to whole capture sets: past it the OLDEST set is + /// dropped entire, instead of the newest capture being truncated topic by topic + /// as it is written. Writing row by row is what produced freeze frames with + /// some topics silently missing and nothing on the wire saying which. + /// @param snapshots Every row of one capture; they share a capture_id + virtual void store_snapshots(const std::vector & snapshots) { + for (const auto & snapshot : snapshots) { + store_snapshot(snapshot); + } + } + /// Store a snapshot captured when a fault was confirmed /// @param snapshot The snapshot data to store virtual void store_snapshot(const SnapshotData & snapshot) = 0; @@ -240,11 +298,35 @@ class FaultStorage { } } - /// Get rosbag file info for a fault + /// The MOST RECENT recording of a fault, or nullopt. + /// + /// A fault can hold several recordings, so "the" recording is a choice: newest + /// wins, because a black box is evidence about the machine you are about to + /// inspect. Implementations must order deterministically - an unordered pick + /// serves an arbitrary recording, which no test catches reliably. /// @param fault_code The fault code to get rosbag for /// @return Rosbag file info if exists, nullopt otherwise virtual std::optional get_rosbag_file(const std::string & fault_code) const = 0; + /// Every recording of a fault, newest first. + virtual std::vector get_rosbag_files(const std::string & fault_code) const = 0; + + /// Every row of one recording - one per fault the recording covers. Backs the + /// bulk-data download and the entity authorization scope check, both of which + /// start from a recording id and need the faults behind it. + virtual std::vector get_rosbag_files_by_recording(const std::string & recording_id) const = 0; + + /// Drop ONE (fault, recording) link. The bag is unlinked only when the removed row + /// was its last reference, so a sibling fault of the same burst keeps it alive. + /// @return true if a row was removed + virtual bool drop_rosbag_link(const std::string & fault_code, const std::string & recording_id) = 0; + + /// Delete one whole recording: every fault's link to it, and the bag. This is the + /// right unit for quota eviction and for a bag that has vanished from disk - both + /// are facts about the recording, not about one fault that happens to reference it. + /// @return number of rows removed + virtual size_t delete_rosbag_recording(const std::string & recording_id) = 0; + /// Delete rosbag file record and the actual file for a fault. Faults from one /// burst can share a recording; the file is unlinked only with the last record /// that references it. @@ -328,20 +410,27 @@ class InMemoryFaultStorage : public FaultStorage { std::vector check_time_based_confirmation(const rclcpp::Time & current_time) override; void set_max_snapshots_per_fault(size_t max_count) override; + void set_retain_snapshots_on_clear(bool retain) override; void store_snapshot(const SnapshotData & snapshot) override; + void store_snapshots(const std::vector & snapshots) override; std::vector get_snapshots(const std::string & fault_code, const std::string & topic_filter = "") const override; void store_freeze_frame(const FreezeFrameData & frame) override; std::optional get_freeze_frame(const std::string & fault_code) const override; + void set_max_rosbags_per_fault(size_t max_count) 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. void store_rosbag_files(const std::vector & infos) override; std::optional get_rosbag_file(const std::string & fault_code) const override; + std::vector get_rosbag_files(const std::string & fault_code) const override; + std::vector get_rosbag_files_by_recording(const std::string & recording_id) const override; bool delete_rosbag_file(const std::string & fault_code) override; + bool drop_rosbag_link(const std::string & fault_code, const std::string & recording_id) override; + size_t delete_rosbag_recording(const std::string & recording_id) override; size_t get_total_rosbag_storage_bytes() const override; std::vector get_all_rosbag_files() const override; std::vector list_rosbags_for_entity(const std::string & entity_fqn) const override; @@ -357,13 +446,35 @@ class InMemoryFaultStorage : public FaultStorage { /// only be unlinked once the last of them is gone. Caller holds mutex_. bool path_shared_with_other_fault(const std::string & file_path, const std::string & fault_code) const; + /// Whether any row at all still references @p file_path. Caller holds mutex_. + bool path_referenced(const std::string & file_path) const; + mutable std::mutex mutex_; std::map faults_; std::vector snapshots_; std::map freeze_frames_; ///< fault_code -> freeze-frame (retained across clear) - std::map rosbag_files_; ///< fault_code -> rosbag info + /// One entry per LINK, mirroring the flat SQLite table rather than a map keyed by + /// fault code - a fault holds several recordings now, and a recording several + /// faults. + /// + /// `seq` is the in-memory twin of SQLite's autoincrement id and is load-bearing, + /// not decoration: a burst stamps ONE created_at_ns across every row it writes, so + /// ties are guaranteed. SQLite breaks them by id; without seq the two backends + /// would order differently and the parity tests would go flaky instead of failing + /// honestly. + struct RosbagRow { + RosbagFileInfo info; + uint64_t seq{0}; + }; + std::vector rosbag_files_; + uint64_t rosbag_seq_{0}; + /// Defaults to 1, the pre-#620 behaviour: a new recording replaces the old one. + /// 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}; DebounceConfig config_; size_t max_snapshots_per_fault_{0}; ///< 0 = unlimited + bool retain_snapshots_on_clear_{false}; }; } // namespace ros2_medkit_fault_manager diff --git a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/snapshot_capture.hpp b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/snapshot_capture.hpp index b14152036..832ca16ce 100644 --- a/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/snapshot_capture.hpp +++ b/src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/snapshot_capture.hpp @@ -88,6 +88,12 @@ struct RosbagConfig { /// (reliable/transient-local where offered) instead of forcing best-effort bool qos_match{true}; + /// Recordings retained per fault code (0 = unlimited, bounded only by + /// max_total_storage_mb). Keep-newest: past the cap the fault's OLDEST recording + /// loses its row, and the bag goes once no fault references it. 1 reproduces the + /// pre-#620 behaviour, where a re-confirm replaced the previous recording. + size_t max_bags_per_fault{1}; + /// If true, delete bag file when fault is cleared bool auto_cleanup{true}; }; @@ -199,14 +205,17 @@ class SnapshotCapture { /// Capture a single topic on-demand (creates temporary subscription) /// On success also records the captured value into @p freeze_frame under the topic key. /// @return true if capture was successful - bool capture_topic_on_demand(const std::string & fault_code, const std::string & topic, - nlohmann::json & freeze_frame); + /// Appends to @p rows rather than storing: a capture is persisted as one set, + /// so the per-fault cap can drop a whole old capture instead of truncating this + /// one topic by topic. + bool capture_topic_on_demand(const std::string & fault_code, const std::string & topic, nlohmann::json & freeze_frame, + std::vector & rows); /// Capture a topic from background cache /// On success also records the cached value into @p freeze_frame under the topic key. /// @return true if data was available in cache bool capture_topic_from_cache(const std::string & fault_code, const std::string & topic, - nlohmann::json & freeze_frame); + nlohmann::json & freeze_frame, std::vector & rows); /// Initialize background subscriptions for all configured topics void init_background_subscriptions(); 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 c60ada868..ebe79e961 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 @@ -63,8 +63,12 @@ class SqliteFaultStorage : public FaultStorage { std::vector check_time_based_confirmation(const rclcpp::Time & current_time) override; void set_max_snapshots_per_fault(size_t max_count) override; + void set_retain_snapshots_on_clear(bool retain) override; + + void set_max_rosbags_per_fault(size_t max_count) override; void store_snapshot(const SnapshotData & snapshot) override; + void store_snapshots(const std::vector & snapshots) override; std::vector get_snapshots(const std::string & fault_code, const std::string & topic_filter = "") const override; @@ -74,7 +78,11 @@ class SqliteFaultStorage : public FaultStorage { 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; + std::vector get_rosbag_files(const std::string & fault_code) const override; + std::vector get_rosbag_files_by_recording(const std::string & recording_id) const override; bool delete_rosbag_file(const std::string & fault_code) override; + bool drop_rosbag_link(const std::string & fault_code, const std::string & recording_id) override; + size_t delete_rosbag_recording(const std::string & recording_id) override; size_t delete_rosbag_files(const std::vector & fault_codes) override; size_t get_total_rosbag_storage_bytes() const override; std::vector get_all_rosbag_files() const override; @@ -91,6 +99,22 @@ class SqliteFaultStorage : public FaultStorage { /// Initialize database schema void initialize_schema(); + /// Whether rosbag_files still carries the legacy column-level UNIQUE on + /// fault_code. Detected from the schema itself (PRAGMA index_list, origin 'u') + /// rather than from a version counter, so a fresh database, a migrated one and + /// one migrated by a later release all answer correctly with no bookkeeping. + bool rosbag_files_has_unique_constraint() const; + + /// Rebuild rosbag_files without the legacy UNIQUE(fault_code), so one fault can + /// hold several recordings. SQLite cannot drop a column constraint in place and + /// CREATE TABLE IF NOT EXISTS is a no-op on an existing database, so this is the + /// documented table-rebuild procedure. Idempotent; no filesystem side effects. + void migrate_rosbag_files_drop_unique(); + + /// Add and backfill recording_id on databases that predate it. Backfill runs in + /// C++ through rosbag_recording_id() so the basename rule has one implementation. + void migrate_rosbag_files_add_recording_id(); + /// 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_. @@ -101,7 +125,10 @@ class SqliteFaultStorage : public FaultStorage { /// store_rosbag_file body without taking mutex_. Caller holds mutex_ and /// unlinks the returned replaced-bag path once the row change is durable. - std::optional store_rosbag_file_locked(const RosbagFileInfo & info); + /// @return file_paths whose last row for this fault the per-fault cap evicted. + /// The caller unlinks each only after the commit, and only if path_referenced() + /// still says nobody holds it. + std::vector store_rosbag_file_locked(const RosbagFileInfo & info); /// Run a plain SQL statement or throw with the SQLite error. Caller holds mutex_. void exec_or_throw(const char * sql); @@ -117,6 +144,10 @@ class SqliteFaultStorage : public FaultStorage { mutable std::mutex mutex_; DebounceConfig config_; size_t max_snapshots_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. + size_t max_rosbags_per_fault_{1}; }; } // namespace ros2_medkit_fault_manager 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 5765b013f..965444c97 100644 --- a/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_manager_node.cpp @@ -69,6 +69,34 @@ std::string validate_fault_code(const std::string & fault_code) { return ""; // Valid } +/// Validate a recording id (the bag directory basename, `fault__`). +/// +/// Same alphabet and the same no-`..` rule as a fault code, because the id ends up +/// as a path segment and a filesystem lookup. The LENGTH bound is different: a +/// recording id is `fault_` + a fault code + `_` + a millisecond stamp, so a +/// maximal fault code produces ~148 characters and would be rejected by the +/// fault-code validator it is derived from. +std::string validate_recording_id(const std::string & recording_id) { + constexpr size_t kMaxRecordingIdLength = kMaxFaultCodeLength + 32; + + if (recording_id.empty()) { + return "recording_id cannot be empty"; + } + if (recording_id.length() > kMaxRecordingIdLength) { + return "recording_id exceeds maximum length of " + std::to_string(kMaxRecordingIdLength); + } + for (char c : recording_id) { + if (!std::isalnum(static_cast(c)) && c != '_' && c != '-' && c != '.') { + return "recording_id contains invalid character '" + std::string(1, c) + + "'. Only alphanumeric, underscore, hyphen, and dot are allowed"; + } + } + if (recording_id.find("..") != std::string::npos) { + return "recording_id cannot contain '..'"; + } + return ""; // Valid +} + } // namespace FaultManagerNode::FaultManagerNode(const rclcpp::NodeOptions & options) : Node("fault_manager", options) { @@ -144,6 +172,13 @@ FaultManagerNode::FaultManagerNode(const rclcpp::NodeOptions & options) : Node(" capture_queue_depth_, capture_queue_full_policy_ == QueueFullPolicy::kRejectNewest ? "reject_newest" : "drop_oldest"); + // Off by default, which is the historical behaviour. Turn it on together with a + // rosbag history: recordings now survive an acknowledgement, so deleting the + // readings captured beside them leaves the fault holding bags whose values are + // gone. The per-fault cap bounds growth either way, so clearing was never the + // only thing keeping the table down. + auto retain_snapshots_on_clear = declare_parameter("snapshots.retain_on_clear", false); + auto max_snapshots = declare_parameter("snapshots.max_per_fault", 10); if (max_snapshots < 0) { RCLCPP_WARN(get_logger(), "snapshots.max_per_fault should be >= 0, got %ld. Disabling limit.", max_snapshots); @@ -159,6 +194,7 @@ FaultManagerNode::FaultManagerNode(const rclcpp::NodeOptions & options) : Node(" // Apply snapshot limit to storage if (max_snapshots > 0) { storage_->set_max_snapshots_per_fault(static_cast(max_snapshots)); + storage_->set_retain_snapshots_on_clear(retain_snapshots_on_clear); } // Create event publisher for SSE streaming @@ -296,6 +332,10 @@ FaultManagerNode::FaultManagerNode(const rclcpp::NodeOptions & options) : Node(" // Initialize rosbag capture if enabled if (snapshot_config.rosbag.enabled) { + // The cap lives in the storage backend, not in RosbagCapture: it has to be + // atomic with the insert, the unlink has to happen after the backend's commit, + // and the "is this bag still referenced" rule is backend-private. + storage_->set_max_rosbags_per_fault(snapshot_config.rosbag.max_bags_per_fault); rosbag_capture_ = std::make_shared(this, storage_.get(), snapshot_config.rosbag, snapshot_config); } @@ -1014,17 +1054,20 @@ void FaultManagerNode::handle_get_fault(const std::shared_ptrget_rosbag_file(request->fault_code); - if (rosbag_info) { + // One entry per recording, newest first. A fault that keeps re-confirming leaves a + // trail of black boxes, and every one of them needs its own addressable id here - + // this list is the only place the fault itself advertises them. + for (const auto & rosbag_info : storage_->get_rosbag_files(request->fault_code)) { ros2_medkit_msgs::msg::Snapshot rosbag_snapshot; rosbag_snapshot.type = ros2_medkit_msgs::msg::Snapshot::TYPE_ROSBAG; - rosbag_snapshot.name = "rosbag_" + request->fault_code; - rosbag_snapshot.bulk_data_id = request->fault_code; - rosbag_snapshot.size_bytes = rosbag_info->size_bytes; - rosbag_snapshot.duration_sec = rosbag_info->duration_sec; - rosbag_snapshot.format = rosbag_info->format; - rosbag_snapshot.captured_at_ns = rosbag_info->created_at_ns; + rosbag_snapshot.name = "rosbag_" + rosbag_info.recording_id; + // The RECORDING, not the fault code: several recordings of one fault would + // otherwise all carry the same id and collapse into one download. + rosbag_snapshot.bulk_data_id = rosbag_info.recording_id; + rosbag_snapshot.size_bytes = rosbag_info.size_bytes; + rosbag_snapshot.duration_sec = rosbag_info.duration_sec; + rosbag_snapshot.format = rosbag_info.format; + rosbag_snapshot.captured_at_ns = rosbag_info.created_at_ns; // Freeze frame fields left empty for rosbag type response->environment_data.snapshots.push_back(rosbag_snapshot); } @@ -1139,6 +1182,19 @@ SnapshotConfig FaultManagerNode::create_snapshot_config() { } config.rosbag.max_buffer_mb = static_cast(max_buffer); + // Recordings kept per fault code. 1 is the pre-#620 behaviour exactly - a new + // recording replaces the old one - so the mechanism ships switched off and any + // regression report is about the plumbing rather than the policy. Note this is a + // FAIRNESS knob more than a depth knob: max_total_storage_mb is the real disk + // bound, and a large per-fault cap lets one flapping fault eat the budget and + // evict every other fault's black box. + int64_t max_bags_per_fault = declare_parameter("snapshots.rosbag.max_bags_per_fault", 1); + if (max_bags_per_fault < 0) { + RCLCPP_WARN(get_logger(), "snapshots.rosbag.max_bags_per_fault must be >= 0. Treating as unlimited"); + max_bags_per_fault = 0; + } + config.rosbag.max_bags_per_fault = static_cast(max_bags_per_fault); + config.rosbag.auto_cleanup = declare_parameter("snapshots.rosbag.auto_cleanup", true); RCLCPP_INFO(get_logger(), @@ -1337,47 +1393,93 @@ void FaultManagerNode::handle_get_snapshots( void FaultManagerNode::handle_get_rosbag(const std::shared_ptr & request, const std::shared_ptr & response) { - // Validate fault_code - std::string validation_error = validate_fault_code(request->fault_code); - if (!validation_error.empty()) { - response->success = false; - response->error_message = validation_error; - return; - } + // Two ways in. recording_id addresses one specific bag and is tried first; + // fault_code keeps working and means "the newest recording of this fault", which + // is what it has always meant back when a fault could only have one. + // + // A caller that cannot tell the two apart - the gateway, holding one URL segment + // that may be either - sets both to the same string and relies on this fallthrough. + // An id that names no recording is therefore not an answer yet, only a miss. + std::optional rosbag_info; + std::vector attached_codes; + std::string subject; + + if (!request->recording_id.empty()) { + // Validation stays a hard failure: a traversal-shaped id is malformed on + // either reading, and falling back would quietly re-admit it as a fault code. + std::string validation_error = validate_recording_id(request->recording_id); + if (!validation_error.empty()) { + response->success = false; + response->error_message = validation_error; + return; + } - // Check if fault exists - auto fault = storage_->get_fault(request->fault_code); - if (!fault) { - response->success = false; - response->error_message = "Fault not found: " + request->fault_code; - return; + const auto rows = storage_->get_rosbag_files_by_recording(request->recording_id); + if (!rows.empty()) { + subject = "recording " + request->recording_id; + rosbag_info = rows.front(); + for (const auto & row : rows) { + attached_codes.push_back(row.fault_code); + } + } else if (request->fault_code.empty()) { + response->success = false; + response->error_message = "No rosbag file available for recording: " + request->recording_id; + return; + } } - // Get rosbag info from storage - auto rosbag_info = storage_->get_rosbag_file(request->fault_code); if (!rosbag_info) { - response->success = false; - response->error_message = "No rosbag file available for fault: " + request->fault_code; - return; + std::string validation_error = validate_fault_code(request->fault_code); + if (!validation_error.empty()) { + response->success = false; + response->error_message = validation_error; + return; + } + subject = "fault " + request->fault_code; + + // Check if fault exists + auto fault = storage_->get_fault(request->fault_code); + if (!fault) { + response->success = false; + response->error_message = "Fault not found: " + request->fault_code; + return; + } + + rosbag_info = storage_->get_rosbag_file(request->fault_code); + if (!rosbag_info) { + response->success = false; + response->error_message = "No rosbag file available for fault: " + request->fault_code; + return; + } + // Report every fault the recording covers, not only the one asked about: the + // caller authorizes the download against this set. + for (const auto & row : storage_->get_rosbag_files_by_recording(rosbag_info->recording_id)) { + attached_codes.push_back(row.fault_code); + } + if (attached_codes.empty()) { + attached_codes.push_back(request->fault_code); + } } // Check if file exists if (!std::filesystem::exists(rosbag_info->file_path)) { response->success = false; response->error_message = "Rosbag file not found on disk: " + rosbag_info->file_path; - // Clean up the stale record - storage_->delete_rosbag_file(request->fault_code); + // Clean up the stale record BY RECORDING. Deleting by fault code would take the + // fault's other, healthy recordings with it because one of them vanished. + storage_->delete_rosbag_recording(rosbag_info->recording_id); return; } response->success = true; response->file_path = rosbag_info->file_path; + response->recording_id = rosbag_info->recording_id; + response->fault_codes = attached_codes; response->format = rosbag_info->format; response->duration_sec = rosbag_info->duration_sec; response->size_bytes = rosbag_info->size_bytes; - RCLCPP_DEBUG(get_logger(), "GetRosbag returned file '%s' for fault '%s'", rosbag_info->file_path.c_str(), - request->fault_code.c_str()); + RCLCPP_DEBUG(get_logger(), "GetRosbag returned file '%s' for %s", rosbag_info->file_path.c_str(), subject.c_str()); } void FaultManagerNode::handle_list_rosbags( @@ -1394,14 +1496,21 @@ void FaultManagerNode::handle_list_rosbags( // Use batch storage API to get all rosbags for this entity auto rosbags = storage_->list_rosbags_for_entity(request->entity_fqn); + std::set reaped; // one cleanup per recording, not per row for (const auto & info : rosbags) { // Skip rosbags whose files no longer exist on disk if (!std::filesystem::exists(info.file_path)) { - storage_->delete_rosbag_file(info.fault_code); + // BY RECORDING: the bytes are gone for every fault that referenced them, so + // leaving the sibling rows would keep charging the quota for nothing. Deleting + // by fault code would instead take that fault's healthy recordings with it. + if (reaped.insert(info.recording_id).second) { + storage_->delete_rosbag_recording(info.recording_id); + } continue; } response->fault_codes.push_back(info.fault_code); + response->recording_ids.push_back(info.recording_id); response->file_paths.push_back(info.file_path); response->formats.push_back(info.format); response->durations_sec.push_back(info.duration_sec); diff --git a/src/ros2_medkit_fault_manager/src/fault_storage.cpp b/src/ros2_medkit_fault_manager/src/fault_storage.cpp index 8d3c0a5d1..6a1a8a5e7 100644 --- a/src/ros2_medkit_fault_manager/src/fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/fault_storage.cpp @@ -20,6 +20,14 @@ namespace ros2_medkit_fault_manager { +std::string rosbag_recording_id(const std::string & file_path) { + std::filesystem::path p(file_path); + if (!p.has_filename()) { + p = p.parent_path(); // tolerate a trailing slash + } + return p.filename().string(); +} + int32_t clamp_debounce_counter(int32_t counter, const DebounceConfig & config) { // Manual min/max rather than std::clamp: well-defined even if a bad config has lo > hi // (sanitize_debounce_config normally prevents that, but the storage layer must never UB). @@ -290,12 +298,16 @@ bool InMemoryFaultStorage::clear_fault(const std::string & fault_code) { return false; } - // Delete associated snapshots when fault is cleared - snapshots_.erase(std::remove_if(snapshots_.begin(), snapshots_.end(), - [&fault_code](const SnapshotData & s) { - return s.fault_code == fault_code; - }), - snapshots_.end()); + // 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 + // them leaves a fault holding bags whose matching values are gone. + if (!retain_snapshots_on_clear_) { + snapshots_.erase(std::remove_if(snapshots_.begin(), snapshots_.end(), + [&fault_code](const SnapshotData & s) { + return s.fault_code == fault_code; + }), + snapshots_.end()); + } it->second.status = ros2_medkit_msgs::msg::Fault::STATUS_CLEARED; return true; @@ -339,21 +351,70 @@ void InMemoryFaultStorage::set_max_snapshots_per_fault(size_t max_count) { max_snapshots_per_fault_ = max_count; } +void InMemoryFaultStorage::set_retain_snapshots_on_clear(bool retain) { + std::lock_guard lock(mutex_); + retain_snapshots_on_clear_ = retain; +} + void InMemoryFaultStorage::store_snapshot(const SnapshotData & snapshot) { + store_snapshots({snapshot}); +} + +void InMemoryFaultStorage::store_snapshots(const std::vector & snapshots) { + if (snapshots.empty()) { + return; + } std::lock_guard lock(mutex_); + + const std::string fault_code = snapshots.front().fault_code; + + // Built beside the live vector and swapped in, the shape store_rosbag_files + // uses: a capture is all-or-nothing, so a throw partway must not leave half of + // it stored. + auto updated = snapshots_; + updated.insert(updated.end(), snapshots.begin(), snapshots.end()); + if (max_snapshots_per_fault_ > 0) { - size_t count = 0; - for (const auto & s : snapshots_) { - if (s.fault_code == snapshot.fault_code) { - ++count; + // Evict whole capture sets, oldest first, until this fault fits. + // + // The old rule counted rows and rejected the NEW row once full, so a capture + // that straddled the cap was stored in part: some topics present, the rest + // silently absent, indistinguishable from "that topic was not publishing". + // Keep-newest also stops this cap from opposing the rosbag one. + const auto rows_for_fault = [&updated, &fault_code]() { + return static_cast(std::count_if(updated.begin(), updated.end(), [&fault_code](const SnapshotData & s) { + return s.fault_code == fault_code; + })); + }; + + while (rows_for_fault() > max_snapshots_per_fault_) { + bool found = false; + int64_t oldest = 0; + for (const auto & s : updated) { + if (s.fault_code == fault_code && (!found || s.capture_id < oldest)) { + oldest = s.capture_id; + found = true; + } + } + if (!found) { + break; + } + const size_t before = updated.size(); + updated.erase(std::remove_if(updated.begin(), updated.end(), + [&fault_code, oldest](const SnapshotData & s) { + return s.fault_code == fault_code && s.capture_id == oldest; + }), + updated.end()); + // One capture can exceed the cap on its own. Dropping every older set and + // still being over means the cap is smaller than this fault's topic count: + // keep the newest capture whole rather than tearing it. + if (updated.size() == before) { + break; } - } - if (count >= max_snapshots_per_fault_) { - // Silent rejection: storage layer has no logger. Callers should log if needed. - return; // Reject new - keep first N inserted snapshots } } - snapshots_.push_back(snapshot); + + snapshots_.swap(updated); } std::vector InMemoryFaultStorage::get_snapshots(const std::string & fault_code, @@ -385,21 +446,15 @@ std::optional InMemoryFaultStorage::get_freeze_frame(const std: return it->second; } -void InMemoryFaultStorage::store_rosbag_file(const RosbagFileInfo & info) { +void InMemoryFaultStorage::set_max_rosbags_per_fault(size_t max_count) { std::lock_guard lock(mutex_); + max_rosbags_per_fault_ = max_count; +} - // Delete existing bag file if present (prevent orphaned files on re-confirm) - auto it = rosbag_files_.find(info.fault_code); - if (it != rosbag_files_.end()) { - if (it->second.file_path != info.file_path && - !path_shared_with_other_fault(it->second.file_path, info.fault_code)) { - std::error_code ec; - std::filesystem::remove_all(it->second.file_path, ec); - // Ignore errors - file may already be deleted - } - } - - rosbag_files_[info.fault_code] = info; +void InMemoryFaultStorage::store_rosbag_file(const RosbagFileInfo & info) { + // Through the batch path deliberately: with a per-fault cap a single store is + // insert + trim, and both must publish together. + store_rosbag_files({info}); } void InMemoryFaultStorage::store_rosbag_files(const std::vector & infos) { @@ -408,32 +463,74 @@ void InMemoryFaultStorage::store_rosbag_files(const std::vector } std::lock_guard lock(mutex_); - // Built beside the live map and swapped in, because swap cannot throw. The caller - // reads a throw here as "no row was written" and removes the recording, so a batch - // that stored half a burst and then threw would leave those rows naming a bag that - // is gone. Copying costs one allocation per stored recording, not per message. + // Built beside the live vector and swapped in, because swap cannot throw. The + // caller reads a throw here as "no row was written" and removes the recording, so + // a batch that stored half a burst and then threw would leave those rows naming a + // bag that is gone. Copying costs one allocation per stored recording, not per + // message. std::vector::swap is noexcept for the default allocator, exactly as + // std::map::swap was. auto updated = rosbag_files_; + uint64_t next_seq = rosbag_seq_; + std::set evicted; + + for (const auto & in : infos) { + RosbagFileInfo row = in; + if (row.recording_id.empty()) { + row.recording_id = rosbag_recording_id(row.file_path); + } + + // Upsert on the (fault, recording) LINK - the same grain as the SQLite unique + // index. Re-storing the same link refreshes it; a link to a different recording + // appends, which is the feature. + auto it = std::find_if(updated.begin(), updated.end(), [&row](const RosbagRow & r) { + return r.info.fault_code == row.fault_code && r.info.file_path == row.file_path; + }); + if (it != updated.end()) { + it->info = row; + } else { + updated.push_back(RosbagRow{row, ++next_seq}); + } + + if (max_rosbags_per_fault_ == 0) { + continue; + } - std::set replaced; - for (const auto & info : infos) { - auto it = updated.find(info.fault_code); - if (it != updated.end() && it->second.file_path != info.file_path) { - replaced.insert(it->second.file_path); + // Keep the newest N recordings of this fault, oldest evicted first - the same + // direction as evict_bags_over_quota, so the two eviction owners never need a + // tiebreak. At N = 1 this is the pre-#620 behaviour exactly. + std::vector mine; + for (size_t i = 0; i < updated.size(); ++i) { + if (updated[i].info.fault_code == row.fault_code) { + mine.push_back(i); + } + } + if (mine.size() <= max_rosbags_per_fault_) { + continue; + } + std::sort(mine.begin(), mine.end(), [&updated](size_t a, size_t b) { + return std::tie(updated[a].info.created_at_ns, updated[a].seq) < + std::tie(updated[b].info.created_at_ns, updated[b].seq); + }); + std::vector doomed(mine.begin(), + mine.begin() + static_cast(mine.size() - max_rosbags_per_fault_)); + std::sort(doomed.rbegin(), doomed.rend()); // descending, so erase stays valid + for (size_t idx : doomed) { + evicted.insert(updated[idx].info.file_path); + updated.erase(updated.begin() + static_cast(idx)); } - updated[info.fault_code] = info; } + rosbag_files_.swap(updated); + rosbag_seq_ = next_seq; // Unlinked only once every row is in, the way the SQLite backend unlinks after its // COMMIT: a throw above must leave the old rows pointing at bags that still exist. - // Sharing is decided on the finished batch, not on the state before it - two faults - // of one burst can have shared the bag being replaced, and checking row by row - // beforehand would find it still referenced by the sibling and leak it. - for (const auto & path : replaced) { - const bool still_referenced = std::any_of(rosbag_files_.begin(), rosbag_files_.end(), [&path](const auto & e) { - return e.second.file_path == path; - }); - if (still_referenced) { + // Referencing is decided on the finished batch, not on the state before it - two + // faults of one burst can share the bag being evicted, and checking row by row + // beforehand would find it still held by a sibling that a later iteration then + // evicts, leaking the directory. + for (const auto & path : evicted) { + if (path_referenced(path)) { continue; } std::error_code ec; @@ -445,37 +542,144 @@ void InMemoryFaultStorage::store_rosbag_files(const std::vector std::optional InMemoryFaultStorage::get_rosbag_file(const std::string & fault_code) const { std::lock_guard lock(mutex_); - auto it = rosbag_files_.find(fault_code); - if (it == rosbag_files_.end()) { + // Newest, matching the SQLite backend's ORDER BY created_at_ns DESC, id DESC. + const RosbagRow * best = nullptr; + for (const auto & row : rosbag_files_) { + if (row.info.fault_code != fault_code) { + continue; + } + if (best == nullptr || std::tie(best->info.created_at_ns, best->seq) < std::tie(row.info.created_at_ns, row.seq)) { + best = &row; + } + } + if (best == nullptr) { return std::nullopt; } - return it->second; + return best->info; +} + +std::vector InMemoryFaultStorage::get_rosbag_files(const std::string & fault_code) const { + std::lock_guard lock(mutex_); + + std::vector mine; + for (const auto & row : rosbag_files_) { + if (row.info.fault_code == fault_code) { + mine.push_back(&row); + } + } + std::sort(mine.begin(), mine.end(), [](const RosbagRow * a, const RosbagRow * b) { + return std::tie(b->info.created_at_ns, b->seq) < std::tie(a->info.created_at_ns, a->seq); // newest first + }); + + std::vector result; + result.reserve(mine.size()); + for (const auto * row : mine) { + result.push_back(row->info); + } + return result; +} + +std::vector +InMemoryFaultStorage::get_rosbag_files_by_recording(const std::string & recording_id) const { + std::lock_guard lock(mutex_); + + std::vector result; + for (const auto & row : rosbag_files_) { + if (row.info.recording_id == recording_id) { + result.push_back(row.info); + } + } + std::sort(result.begin(), result.end(), [](const RosbagFileInfo & a, const RosbagFileInfo & b) { + return a.fault_code < b.fault_code; + }); + return result; } bool InMemoryFaultStorage::delete_rosbag_file(const std::string & fault_code) { std::lock_guard lock(mutex_); - auto it = rosbag_files_.find(fault_code); - if (it == rosbag_files_.end()) { + // ALL recordings of this fault. Used by auto_cleanup on clear, where dropping the + // fault's whole black-box history is the intent. + std::set touched; + const size_t before = rosbag_files_.size(); + for (auto it = rosbag_files_.begin(); it != rosbag_files_.end();) { + if (it->info.fault_code == fault_code) { + touched.insert(it->info.file_path); + it = rosbag_files_.erase(it); + } else { + ++it; + } + } + if (rosbag_files_.size() == before) { return false; } - const std::string file_path = it->second.file_path; + for (const auto & path : touched) { + if (path_referenced(path)) { + continue; // a sibling fault of the burst still holds it + } + std::error_code ec; + std::filesystem::remove_all(path, ec); + // Ignore errors - file may already be deleted + } + return true; +} + +bool InMemoryFaultStorage::drop_rosbag_link(const std::string & fault_code, const std::string & recording_id) { + std::lock_guard lock(mutex_); + + std::string path; + auto it = std::find_if(rosbag_files_.begin(), rosbag_files_.end(), [&](const RosbagRow & r) { + return r.info.fault_code == fault_code && r.info.recording_id == recording_id; + }); + if (it == rosbag_files_.end()) { + return false; + } + path = it->info.file_path; rosbag_files_.erase(it); - // Try to delete the actual file, unless a sibling fault still points at it - if (!path_shared_with_other_fault(file_path, fault_code)) { + if (!path_referenced(path)) { std::error_code ec; - std::filesystem::remove_all(file_path, ec); - // Ignore errors - file may already be deleted + std::filesystem::remove_all(path, ec); } return true; } +size_t InMemoryFaultStorage::delete_rosbag_recording(const std::string & recording_id) { + std::lock_guard lock(mutex_); + + std::set touched; + size_t removed = 0; + for (auto it = rosbag_files_.begin(); it != rosbag_files_.end();) { + if (it->info.recording_id == recording_id) { + touched.insert(it->info.file_path); + it = rosbag_files_.erase(it); + ++removed; + } else { + ++it; + } + } + + for (const auto & path : touched) { + if (path_referenced(path)) { + continue; + } + std::error_code ec; + std::filesystem::remove_all(path, ec); + } + return removed; +} + bool InMemoryFaultStorage::path_shared_with_other_fault(const std::string & file_path, const std::string & fault_code) const { - return std::any_of(rosbag_files_.begin(), rosbag_files_.end(), [&](const auto & entry) { - return entry.first != fault_code && entry.second.file_path == file_path; + return std::any_of(rosbag_files_.begin(), rosbag_files_.end(), [&](const RosbagRow & row) { + return row.info.fault_code != fault_code && row.info.file_path == file_path; + }); +} + +bool InMemoryFaultStorage::path_referenced(const std::string & file_path) const { + return std::any_of(rosbag_files_.begin(), rosbag_files_.end(), [&](const RosbagRow & row) { + return row.info.file_path == file_path; }); } @@ -488,9 +692,9 @@ size_t InMemoryFaultStorage::get_total_rosbag_storage_bytes() const { // finalised, so take the largest - matching the SQLite backend's MAX() and // never under-reporting what is on disk. std::map bytes_per_path; - for (const auto & [code, info] : rosbag_files_) { - auto & bytes = bytes_per_path[info.file_path]; - bytes = std::max(bytes, info.size_bytes); + for (const auto & row : rosbag_files_) { + auto & bytes = bytes_per_path[row.info.file_path]; + bytes = std::max(bytes, row.info.size_bytes); } size_t total = 0; @@ -503,36 +707,51 @@ size_t InMemoryFaultStorage::get_total_rosbag_storage_bytes() const { std::vector InMemoryFaultStorage::get_all_rosbag_files() const { std::lock_guard lock(mutex_); - std::vector result; - result.reserve(rosbag_files_.size()); - for (const auto & [code, info] : rosbag_files_) { - result.push_back(info); + std::vector rows; + rows.reserve(rosbag_files_.size()); + for (const auto & row : rosbag_files_) { + rows.push_back(&row); } - // Sort by creation time (oldest first) - std::sort(result.begin(), result.end(), [](const RosbagFileInfo & a, const RosbagFileInfo & b) { - return a.created_at_ns < b.created_at_ns; + // Oldest first, seq breaking the created_at_ns ties a burst guarantees - the + // SQLite side orders by created_at_ns ASC, id ASC for the same reason. + std::sort(rows.begin(), rows.end(), [](const RosbagRow * a, const RosbagRow * b) { + return std::tie(a->info.created_at_ns, a->seq) < std::tie(b->info.created_at_ns, b->seq); }); + std::vector result; + result.reserve(rows.size()); + for (const auto * row : rows) { + result.push_back(row->info); + } return result; } std::vector InMemoryFaultStorage::list_rosbags_for_entity(const std::string & entity_fqn) const { std::lock_guard lock(mutex_); - std::vector result; - - for (const auto & [fault_code, rosbag_info] : rosbag_files_) { + std::vector rows; + for (const auto & row : rosbag_files_) { // Check if any of the fault's reporting sources contain this entity - auto fault_it = faults_.find(fault_code); - if (fault_it != faults_.end()) { - const auto & fault_state = fault_it->second; - if (fault_state.reporting_sources.find(entity_fqn) != fault_state.reporting_sources.end()) { - result.push_back(rosbag_info); - } + auto fault_it = faults_.find(row.info.fault_code); + if (fault_it == faults_.end()) { + continue; + } + const auto & fault_state = fault_it->second; + if (fault_state.reporting_sources.find(entity_fqn) != fault_state.reporting_sources.end()) { + rows.push_back(&row); } } + std::sort(rows.begin(), rows.end(), [](const RosbagRow * a, const RosbagRow * b) { + return std::tie(b->info.created_at_ns, b->seq) < std::tie(a->info.created_at_ns, a->seq); // newest first + }); + + std::vector result; + result.reserve(rows.size()); + for (const auto * row : rows) { + result.push_back(row->info); + } return result; } diff --git a/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp b/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp index d83fa5ac7..7a475b230 100644 --- a/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp +++ b/src/ros2_medkit_fault_manager/src/rosbag_capture.cpp @@ -563,6 +563,7 @@ void RosbagCapture::on_fault_confirmed(const std::string & fault_code) { RosbagFileInfo info; info.fault_code = fault_code; + info.recording_id = rosbag_recording_id(bag_path); info.file_path = bag_path; info.format = config_.format; // The span the recording was open, not the configured window: a buffer that @@ -590,6 +591,19 @@ void RosbagCapture::on_fault_cleared(const std::string & fault_code) { return; } + // Acknowledging a fault must not destroy a history the operator asked to keep. + // auto_cleanup deletes EVERY recording of the fault, so with a cap above one the + // first acknowledgement wiped the whole trail that max_bags_per_fault had just + // been raised to collect - one default silently cancelling another. When a + // history is configured the cap governs retention and acknowledgement leaves the + // evidence alone; at the default cap of 1 this is exactly the old behaviour. + if (config_.max_bags_per_fault != 1) { + RCLCPP_DEBUG(node_->get_logger(), + "Fault '%s' cleared; keeping its recordings because max_bags_per_fault is %zu, not 1", + fault_code.c_str(), config_.max_bags_per_fault); + return; + } + // A fault of the in-flight burst has no row yet: drop it from the recording // state so the finalize never writes one. A leftover row for a cleared fault // would keep the shared bag referenced forever and break the burst's cleanup. @@ -1276,20 +1290,21 @@ std::vector RosbagCapture::evict_bags_over_quota(FaultStorage * sto return {}; } - // get_all_rosbag_files() is one row per fault, and a burst of correlated faults - // shares one recording. Group the rows back into bags first: deleting a single - // row of a shared bag leaves the directory on disk (a sibling still points at - // it), so freeing its bytes per row would drop the running total below the real - // one and stop the eviction while the quota is still blown. + // get_all_rosbag_files() is one row per (fault, recording) link: a burst of + // correlated faults shares one recording, and one fault can hold several. Group + // the rows back into bags first: deleting a single row of a shared bag leaves the + // directory on disk (a sibling still points at it), so freeing its bytes per row + // would drop the running total below the real one and stop the eviction while the + // quota is still blown. std::vector paths_oldest_first; - std::unordered_map> codes_by_path; + std::unordered_map recording_by_path; std::unordered_map bytes_by_path; for (const auto & bag : storage->get_all_rosbag_files()) { - auto & codes = codes_by_path[bag.file_path]; - if (codes.empty()) { + // First row for this path decides its position in the oldest-first order; the + // rows arrive sorted, so that is the recording's own age. + if (recording_by_path.emplace(bag.file_path, bag.recording_id).second) { paths_oldest_first.push_back(bag.file_path); } - codes.push_back(bag.fault_code); // Mirror the storage accounting, which takes the largest row per path. auto & bytes = bytes_by_path[bag.file_path]; bytes = std::max(bytes, bag.size_bytes); @@ -1301,10 +1316,13 @@ std::vector RosbagCapture::evict_bags_over_quota(FaultStorage * sto break; } - // One call per bag: the SQLite backend removes the whole burst's rows in a - // single transaction and unlinks the directory after the commit, so a crash - // mid-eviction cannot leave rows pointing at a removed bag. - storage->delete_rosbag_files(codes_by_path[path]); + // By RECORDING, not by fault code. The unit being evicted is a bag, and a + // fault can now hold several: deleting by code would take this fault's other, + // newer recordings with it, and their directories would never be unlinked + // because their paths were never collected - invisible to the quota from then + // on. One call per bag, so the SQLite backend still removes the whole burst's + // rows in one transaction and unlinks after the commit. + storage->delete_rosbag_recording(recording_by_path[path]); // Saturate rather than wrap: the quota must never be satisfied by underflow. current_bytes -= std::min(current_bytes, bytes_by_path[path]); evicted.push_back(path); @@ -1515,6 +1533,7 @@ void RosbagCapture::finalize_post_fault_recording() { size_t bag_size = calculate_bag_size(bag_path); RosbagFileInfo info; + info.recording_id = rosbag_recording_id(bag_path); info.file_path = bag_path; info.format = config_.format; // The real span of the recording, not the configured pre+post window. A diff --git a/src/ros2_medkit_fault_manager/src/snapshot_capture.cpp b/src/ros2_medkit_fault_manager/src/snapshot_capture.cpp index 17a00f173..458896094 100644 --- a/src/ros2_medkit_fault_manager/src/snapshot_capture.cpp +++ b/src/ros2_medkit_fault_manager/src/snapshot_capture.cpp @@ -13,6 +13,7 @@ // limitations under the License. #include "ros2_medkit_fault_manager/snapshot_capture.hpp" +#include #include #include @@ -147,15 +148,21 @@ void SnapshotCapture::capture(const std::string & fault_code) { // Accumulate a compact dict of captured topic values alongside the per-topic snapshots. nlohmann::json freeze_frame = nlohmann::json::object(); + // One id for the whole capture, minted here so every row of this confirmation + // shares it. Monotonic rather than a clock read: the rows are written seconds + // apart under load and a wall clock is not guaranteed to move forward. + static std::atomic capture_seq{0}; + const int64_t capture_id = ++capture_seq; + std::vector rows; size_t captured_count = 0; for (const auto & topic : topics) { bool success = false; // Entity-default topics are not known at construction, so the background // cache never holds them - always sample those on demand. if (config_.background_capture && !entity_scoped) { - success = capture_topic_from_cache(fault_code, topic, freeze_frame); + success = capture_topic_from_cache(fault_code, topic, freeze_frame, rows); } else { - success = capture_topic_on_demand(fault_code, topic, freeze_frame); + success = capture_topic_on_demand(fault_code, topic, freeze_frame, rows); } if (success) { @@ -166,6 +173,14 @@ void SnapshotCapture::capture(const std::string & fault_code) { RCLCPP_INFO(node_->get_logger(), "Captured %zu/%zu snapshots for fault '%s'", captured_count, topics.size(), fault_code.c_str()); + // One write for the whole capture. Storing topic by topic let the per-fault cap + // reject the tail of a capture, leaving a freeze frame with holes and nothing + // saying which values were dropped. + for (auto & row : rows) { + row.capture_id = capture_id; + } + storage_->store_snapshots(rows); + // Persist the compact freeze-frame keyed by fault_code (retained across clear_fault). // Only reached for faults with a configured capture set (unconfigured codes returned // early above and get no row). If nothing was publishing at confirmation time the row @@ -301,7 +316,7 @@ std::vector SnapshotCapture::resolve_entity_topics(const std::strin } bool SnapshotCapture::capture_topic_on_demand(const std::string & fault_code, const std::string & topic, - nlohmann::json & freeze_frame) { + nlohmann::json & freeze_frame, std::vector & rows) { // Get topic type std::string msg_type = get_topic_type(topic); if (msg_type.empty()) { @@ -411,7 +426,7 @@ bool SnapshotCapture::capture_topic_on_demand(const std::string & fault_code, co snapshot.data = json_data.dump(); snapshot.captured_at_ns = get_wall_clock_ns(); - storage_->store_snapshot(snapshot); + rows.push_back(std::move(snapshot)); // Record the value into the compact freeze-frame dict under the topic key. freeze_frame[topic] = std::move(json_data); @@ -431,7 +446,7 @@ bool SnapshotCapture::capture_topic_on_demand(const std::string & fault_code, co } bool SnapshotCapture::capture_topic_from_cache(const std::string & fault_code, const std::string & topic, - nlohmann::json & freeze_frame) { + nlohmann::json & freeze_frame, std::vector & rows) { std::lock_guard lock(cache_mutex_); auto it = message_cache_.find(topic); @@ -450,7 +465,7 @@ bool SnapshotCapture::capture_topic_from_cache(const std::string & fault_code, c snapshot.data = cached.data; snapshot.captured_at_ns = cached.timestamp_ns; - storage_->store_snapshot(snapshot); + rows.push_back(std::move(snapshot)); // Record the cached value into the compact freeze-frame dict. The cache holds a serialized // JSON string; parse it back so the frame nests structured values (fall back to the raw 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 e49423bdf..b76417c4e 100644 --- a/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp +++ b/src/ros2_medkit_fault_manager/src/sqlite_fault_storage.cpp @@ -243,19 +243,49 @@ void SqliteFaultStorage::initialize_schema() { throw std::runtime_error("Failed to create freeze_frames table: " + error); } - // Create rosbag_files table for storing time-window bag file metadata + // Migration: snapshots gained capture_id, which groups the rows of one capture. + // Without it the per-fault cap could not tell where a capture ended and trimmed + // by row, storing a confirmation's values in part. Rows written before it read + // as capture 0 - one legacy set, which is how they behaved anyway. + { + bool has_capture_id = false; + SqliteStatement info(db_, "PRAGMA table_info(snapshots)"); + while (info.step() == SQLITE_ROW) { + if (info.column_text(1) == "capture_id") { + has_capture_id = true; + break; + } + } + if (!has_capture_id) { + if (sqlite3_exec(db_, "ALTER TABLE snapshots ADD COLUMN capture_id INTEGER NOT NULL DEFAULT 0", 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 capture_id 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. + // + // House rule, learned the hard way here: uniqueness is expressed with + // CREATE UNIQUE INDEX, never as a column constraint. The original + // `fault_code TEXT NOT NULL UNIQUE` could not be dropped with ALTER TABLE and + // forced the full table rebuild in migrate_rosbag_files_drop_unique() below. + // A named index would have been one DROP INDEX. const char * create_rosbag_files_table_sql = R"( CREATE TABLE IF NOT EXISTS rosbag_files ( id INTEGER PRIMARY KEY AUTOINCREMENT, - fault_code TEXT NOT NULL UNIQUE, + fault_code TEXT NOT NULL, + recording_id TEXT NOT NULL DEFAULT '', file_path TEXT NOT NULL, format TEXT NOT NULL, duration_sec REAL NOT NULL, size_bytes INTEGER NOT NULL, created_at_ns INTEGER NOT NULL ); - CREATE INDEX IF NOT EXISTS idx_rosbag_files_fault_code ON rosbag_files(fault_code); - CREATE INDEX IF NOT EXISTS idx_rosbag_files_created_at ON rosbag_files(created_at_ns); )"; if (sqlite3_exec(db_, create_rosbag_files_table_sql, nullptr, nullptr, &err_msg) != SQLITE_OK) { @@ -263,6 +293,186 @@ void SqliteFaultStorage::initialize_schema() { sqlite3_free(err_msg); throw std::runtime_error("Failed to create rosbag_files table: " + error); } + + // Order matters: drop the legacy constraint before adding the column, so the + // rebuild only ever has to copy the old column set. + migrate_rosbag_files_drop_unique(); + migrate_rosbag_files_add_recording_id(); + + // Indexes last, so they serve a fresh table and a rebuilt one alike. + // + // idx_rosbag_files_path is capability, not tuning: path_referenced() and + // path_shared_with_other_fault() scan on file_path on every delete, against a + // table that now holds N rows per fault instead of one. + const char * create_rosbag_files_indexes_sql = R"( + CREATE INDEX IF NOT EXISTS idx_rosbag_files_fault_code ON rosbag_files(fault_code); + CREATE INDEX IF NOT EXISTS idx_rosbag_files_created_at ON rosbag_files(created_at_ns); + CREATE INDEX IF NOT EXISTS idx_rosbag_files_fault_created ON rosbag_files(fault_code, created_at_ns, id); + CREATE INDEX IF NOT EXISTS idx_rosbag_files_recording ON rosbag_files(recording_id); + CREATE INDEX IF NOT EXISTS idx_rosbag_files_path ON rosbag_files(file_path); + )"; + + if (sqlite3_exec(db_, create_rosbag_files_indexes_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 rosbag_files indexes: " + error); + } + + // The grain the old UNIQUE was reaching for, expressed correctly. Keyed on + // file_path rather than recording_id: file_path is the identity of the thing on + // disk. Two bags in different directories sharing a basename would, under a + // recording_id key, make the second store silently REPLACE the first - a lost + // recording. Under a file_path key the same collision is only a mislabelled + // download. Deduplicate first, because a legacy database rebuilt above may hold + // rows this index would reject. + if (sqlite3_exec(db_, + "DELETE FROM rosbag_files WHERE id NOT IN " + "(SELECT MAX(id) FROM rosbag_files GROUP BY fault_code, file_path)", + 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 deduplicate rosbag_files rows: " + error); + } + + if (sqlite3_exec(db_, + "CREATE UNIQUE INDEX IF NOT EXISTS idx_rosbag_files_fault_path " + "ON rosbag_files(fault_code, file_path)", + 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 rosbag_files unique index: " + error); + } +} + +bool SqliteFaultStorage::rosbag_files_has_unique_constraint() const { + // origin 'u' == an index SQLite created for a UNIQUE table/column constraint, + // which ALTER TABLE cannot drop. 'c' == CREATE [UNIQUE] INDEX, which it can. + // The INTEGER PRIMARY KEY AUTOINCREMENT is a rowid alias and produces no + // index_list row at all, so it cannot be mistaken for one, and our own + // idx_rosbag_files_fault_path reports 'c'. That asymmetry is the whole reason + // this probe works without a version counter to maintain. + SqliteStatement info(db_, "PRAGMA index_list(rosbag_files)"); + while (info.step() == SQLITE_ROW) { + if (info.column_text(3) == "u") { + return true; + } + } + return false; +} + +void SqliteFaultStorage::migrate_rosbag_files_drop_unique() { + if (!rosbag_files_has_unique_constraint()) { + return; // fresh table, or already migrated - safe to re-run on every open + } + + // SQLite's documented table-rebuild procedure. No filesystem side effects: the + // migration only moves rows, and it must NOT interpret a row whose bag is gone + // as garbage - with the default storage_path the bags live in the system temp + // directory and legitimately vanish across a reboot. + char * err_msg = nullptr; + const auto exec = [this, &err_msg](const char * sql, const char * what) { + if (sqlite3_exec(db_, sql, nullptr, nullptr, &err_msg) != SQLITE_OK) { + std::string error = err_msg ? err_msg : "Unknown error"; + sqlite3_free(err_msg); + err_msg = nullptr; + throw std::runtime_error(std::string("rosbag_files migration failed (") + what + "): " + error); + } + }; + + exec("BEGIN IMMEDIATE", "begin"); + try { + exec( + "CREATE TABLE rosbag_files_new (" + " id INTEGER PRIMARY KEY AUTOINCREMENT," + " fault_code TEXT NOT NULL," + " recording_id TEXT NOT NULL DEFAULT ''," + " file_path TEXT NOT NULL," + " format TEXT NOT NULL," + " duration_sec REAL NOT NULL," + " size_bytes INTEGER NOT NULL," + " created_at_ns INTEGER NOT NULL)", + "create new table"); + + // ORDER BY id so the fresh autoincrement ids preserve the old relative order. + // Every read below breaks created_at_ns ties by id, and a burst writes one + // created_at_ns for all its rows, so that tiebreak is load-bearing. + // recording_id stays '' here and is filled by the next migration step, which + // also has to serve a database that only lacks the column. + exec( + "INSERT INTO rosbag_files_new " + "(fault_code, recording_id, file_path, format, duration_sec, size_bytes, created_at_ns) " + "SELECT fault_code, '', file_path, format, duration_sec, size_bytes, created_at_ns " + "FROM rosbag_files ORDER BY id", + "copy rows"); + + exec("DROP TABLE rosbag_files", "drop old table"); + exec("ALTER TABLE rosbag_files_new RENAME TO rosbag_files", "rename"); + exec("COMMIT", "commit"); + } catch (...) { + sqlite3_exec(db_, "ROLLBACK", nullptr, nullptr, nullptr); + throw; + } +} + +void SqliteFaultStorage::migrate_rosbag_files_add_recording_id() { + bool has_recording_id = false; + { + SqliteStatement info(db_, "PRAGMA table_info(rosbag_files)"); + while (info.step() == SQLITE_ROW) { + if (info.column_text(1) == "recording_id") { + has_recording_id = true; + break; + } + } + } + + char * err_msg = nullptr; + if (!has_recording_id) { + if (sqlite3_exec(db_, "ALTER TABLE rosbag_files ADD COLUMN recording_id 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 recording_id column: " + error); + } + } + + // Backfill in C++ rather than SQL. SQLite has no basename function, and the + // pure-SQL substitute is an unreadable nest of rtrim/replace that is correct only + // by a slash-counting argument. Going through rosbag_recording_id() means the + // invariant recording_id == basename(file_path) holds by construction, because + // new inserts call the same function. + std::vector> pending; + { + SqliteStatement select(db_, "SELECT id, file_path FROM rosbag_files WHERE recording_id = ''"); + while (select.step() == SQLITE_ROW) { + pending.emplace_back(select.column_int64(0), select.column_text(1)); + } + } + if (pending.empty()) { + return; + } + + if (sqlite3_exec(db_, "BEGIN IMMEDIATE", 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 begin recording_id backfill: " + error); + } + try { + for (const auto & [row_id, file_path] : pending) { + SqliteStatement update(db_, "UPDATE rosbag_files SET recording_id = ? WHERE id = ?"); + update.bind_text(1, rosbag_recording_id(file_path)); + update.bind_int64(2, row_id); + update.step(); + } + if (sqlite3_exec(db_, "COMMIT", 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 commit recording_id backfill: " + error); + } + } catch (...) { + sqlite3_exec(db_, "ROLLBACK", nullptr, nullptr, nullptr); + throw; + } } std::vector SqliteFaultStorage::parse_json_array(const std::string & json_str) { @@ -690,10 +900,15 @@ bool SqliteFaultStorage::clear_fault(const std::string & fault_code) { std::lock_guard lock(mutex_); // Delete associated snapshots when fault is cleared - SqliteStatement delete_snapshots(db_, "DELETE FROM snapshots WHERE fault_code = ?"); - delete_snapshots.bind_text(1, fault_code); - if (delete_snapshots.step() != SQLITE_DONE) { - throw std::runtime_error(std::string("Failed to delete snapshots: ") + sqlite3_errmsg(db_)); + // 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 + // them leaves a fault holding bags whose matching values are gone. + if (!retain_snapshots_on_clear_) { + SqliteStatement delete_snapshots(db_, "DELETE FROM snapshots WHERE fault_code = ?"); + delete_snapshots.bind_text(1, fault_code); + if (delete_snapshots.step() != SQLITE_DONE) { + throw std::runtime_error(std::string("Failed to delete snapshots: ") + sqlite3_errmsg(db_)); + } } SqliteStatement stmt(db_, "UPDATE faults SET status = ? WHERE fault_code = ?"); @@ -813,32 +1028,89 @@ void SqliteFaultStorage::set_max_snapshots_per_fault(size_t max_count) { max_snapshots_per_fault_ = max_count; } +void SqliteFaultStorage::set_retain_snapshots_on_clear(bool retain) { + std::lock_guard lock(mutex_); + retain_snapshots_on_clear_ = retain; +} + void SqliteFaultStorage::store_snapshot(const SnapshotData & snapshot) { + store_snapshots({snapshot}); +} + +void SqliteFaultStorage::store_snapshots(const std::vector & snapshots) { + if (snapshots.empty()) { + return; + } std::lock_guard lock(mutex_); - // Check snapshot limit per fault (reject-new strategy: keep earliest) - if (max_snapshots_per_fault_ > 0) { - SqliteStatement count_stmt(db_, "SELECT COUNT(*) FROM snapshots WHERE fault_code = ?"); - count_stmt.bind_text(1, snapshot.fault_code); - if (count_stmt.step() == SQLITE_ROW && - count_stmt.column_int64(0) >= static_cast(max_snapshots_per_fault_)) { - // Silent rejection: storage layer has no logger. Callers should log if needed. - return; // Reject new - keep first N inserted snapshots + const std::string & fault_code = snapshots.front().fault_code; + + // One transaction for the whole capture: a capture is all-or-nothing, and the + // old row-at-a-time path could leave a confirmation's values half stored. + exec_or_throw("BEGIN IMMEDIATE"); + try { + { + SqliteStatement stmt(db_, + "INSERT INTO snapshots (fault_code, topic, message_type, data, captured_at_ns, capture_id) " + "VALUES (?, ?, ?, ?, ?, ?)"); + for (const auto & snapshot : snapshots) { + stmt.reset(); + stmt.bind_text(1, snapshot.fault_code); + stmt.bind_text(2, snapshot.topic); + stmt.bind_text(3, snapshot.message_type); + stmt.bind_text(4, snapshot.data); + stmt.bind_int64(5, snapshot.captured_at_ns); + stmt.bind_int64(6, snapshot.capture_id); + if (stmt.step() != SQLITE_DONE) { + throw std::runtime_error(std::string("Failed to store snapshot: ") + sqlite3_errmsg(db_)); + } + } } - } - SqliteStatement stmt(db_, - "INSERT INTO snapshots (fault_code, topic, message_type, data, captured_at_ns) " - "VALUES (?, ?, ?, ?, ?)"); + if (max_snapshots_per_fault_ > 0) { + // Trim whole capture sets, oldest first, until the fault fits. The old rule + // counted rows and rejected the NEW row once full, so a capture straddling + // the cap was stored in part - some topics present, the rest silently gone, + // indistinguishable from "that topic was not publishing". Keep-newest also + // stops this cap from opposing the rosbag one. + // + // The newest capture is never trimmed: if it alone exceeds the cap, the cap + // is smaller than this fault's topic count and tearing it would be the very + // thing being fixed. + SqliteStatement newest(db_, "SELECT MAX(capture_id) FROM snapshots WHERE fault_code = ?"); + newest.bind_text(1, fault_code); + int64_t newest_capture = 0; + if (newest.step() == SQLITE_ROW) { + newest_capture = newest.column_int64(0); + } - stmt.bind_text(1, snapshot.fault_code); - stmt.bind_text(2, snapshot.topic); - stmt.bind_text(3, snapshot.message_type); - stmt.bind_text(4, snapshot.data); - stmt.bind_int64(5, snapshot.captured_at_ns); + SqliteStatement trim(db_, + "DELETE FROM snapshots WHERE fault_code = ?1 AND capture_id = " + "(SELECT MIN(capture_id) FROM snapshots WHERE fault_code = ?1) " + "AND capture_id <> ?2"); + SqliteStatement count(db_, "SELECT COUNT(*) FROM snapshots WHERE fault_code = ?"); + while (true) { + count.reset(); + count.bind_text(1, fault_code); + if (count.step() != SQLITE_ROW || static_cast(count.column_int64(0)) <= max_snapshots_per_fault_) { + break; + } + trim.reset(); + trim.bind_text(1, fault_code); + trim.bind_int64(2, newest_capture); + if (trim.step() != SQLITE_DONE) { + throw std::runtime_error(std::string("Failed to trim snapshots: ") + sqlite3_errmsg(db_)); + } + if (sqlite3_changes(db_) == 0) { + break; // only the newest capture is left and it is over on its own + } + } + } - if (stmt.step() != SQLITE_DONE) { - throw std::runtime_error(std::string("Failed to store snapshot: ") + sqlite3_errmsg(db_)); + exec_or_throw("COMMIT"); + } catch (...) { + sqlite3_exec(db_, "ROLLBACK", nullptr, nullptr, nullptr); + throw; } } @@ -849,12 +1121,15 @@ std::vector SqliteFaultStorage::get_snapshots(const std::string & std::vector result; std::string sql = - "SELECT fault_code, topic, message_type, data, captured_at_ns FROM snapshots WHERE fault_code " + "SELECT fault_code, topic, message_type, data, captured_at_ns, capture_id FROM snapshots WHERE fault_code " "= ?"; if (!topic_filter.empty()) { sql += " AND topic = ?"; } - sql += " ORDER BY captured_at_ns DESC"; + // capture_id before the timestamp: the rows of one capture are written seconds + // apart under load and their timestamps interleave with a neighbouring capture's, + // so ordering by time alone splits a set the reader then cannot regroup. + sql += " ORDER BY capture_id DESC, captured_at_ns DESC"; SqliteStatement stmt(db_, sql.c_str()); stmt.bind_text(1, fault_code); @@ -869,6 +1144,7 @@ std::vector SqliteFaultStorage::get_snapshots(const std::string & snapshot.message_type = stmt.column_text(2); snapshot.data = stmt.column_text(3); snapshot.captured_at_ns = stmt.column_int64(4); + snapshot.capture_id = stmt.column_int64(5); result.push_back(snapshot); } @@ -917,13 +1193,16 @@ void SqliteFaultStorage::exec_or_throw(const char * sql) { } } -void SqliteFaultStorage::store_rosbag_file(const RosbagFileInfo & info) { +void SqliteFaultStorage::set_max_rosbags_per_fault(size_t max_count) { std::lock_guard lock(mutex_); - const auto replaced = store_rosbag_file_locked(info); - if (replaced) { - std::error_code ec; - std::filesystem::remove_all(*replaced, ec); - } + max_rosbags_per_fault_ = max_count; +} + +void SqliteFaultStorage::store_rosbag_file(const RosbagFileInfo & info) { + // Routed through the batch path deliberately: with a per-fault cap a single + // store is insert + trim, i.e. several statements that must share one + // transaction and one post-commit unlink pass. + store_rosbag_files({info}); } void SqliteFaultStorage::store_rosbag_files(const std::vector & infos) { @@ -933,71 +1212,248 @@ void SqliteFaultStorage::store_rosbag_files(const std::vector & std::lock_guard lock(mutex_); // One transaction for the whole burst: a crash mid-store must not leave some - // faults of the shared recording without their lookup row. Replaced bags are - // unlinked only after COMMIT - a ROLLBACK resurrects the old rows, which must - // keep pointing at bags that still exist. - std::vector replaced; + // faults of the shared recording without their lookup row. Evicted bags are + // unlinked only after COMMIT - a ROLLBACK resurrects the rows, which must keep + // pointing at bags that still exist. + std::vector evicted; exec_or_throw("BEGIN IMMEDIATE"); try { for (const auto & info : infos) { - if (auto old_path = store_rosbag_file_locked(info)) { - replaced.push_back(*std::move(old_path)); - } + auto paths = store_rosbag_file_locked(info); + evicted.insert(evicted.end(), std::make_move_iterator(paths.begin()), std::make_move_iterator(paths.end())); } exec_or_throw("COMMIT"); } catch (...) { sqlite3_exec(db_, "ROLLBACK", nullptr, nullptr, nullptr); throw; } - for (const auto & path : replaced) { + + // Referencing is re-checked on the committed state, not on the state each + // eviction saw: two faults of one burst can link the same bag, and a row-by-row + // check inside the loop would find it still held by a sibling that a later + // iteration then evicts, leaking the directory. + std::set unique_paths(evicted.begin(), evicted.end()); + for (const auto & path : unique_paths) { + if (path_referenced(path)) { + continue; + } std::error_code ec; std::filesystem::remove_all(path, ec); } } -std::optional SqliteFaultStorage::store_rosbag_file_locked(const RosbagFileInfo & info) { - // A re-confirm replaces the row; report the orphaned old bag to the caller, - // which unlinks it once the row change is durable. - std::optional replaced; - { - SqliteStatement query_stmt(db_, "SELECT file_path FROM rosbag_files WHERE fault_code = ?"); - query_stmt.bind_text(1, info.fault_code); - if (query_stmt.step() == SQLITE_ROW) { - std::string old_path = query_stmt.column_text(0); - if (old_path != info.file_path && !path_shared_with_other_fault(old_path, info.fault_code)) { - replaced = std::move(old_path); - } - } - } - - // Use INSERT OR REPLACE to handle updates (fault_code is UNIQUE) +std::vector SqliteFaultStorage::store_rosbag_file_locked(const RosbagFileInfo & info) { + RosbagFileInfo row = info; + if (row.recording_id.empty()) { + row.recording_id = rosbag_recording_id(row.file_path); + } + + // Upserts on idx_rosbag_files_fault_path, i.e. on the (fault, recording) link. + // Re-storing the SAME link refreshes it; a link to a DIFFERENT recording is a new + // row now, which is the feature. Nothing is unlinked here - byte lifetime is the + // cap's business below, and the caller's, after the commit. + // + // ON CONFLICT DO UPDATE, not INSERT OR REPLACE: the latter deletes the row and + // inserts a fresh one, so a refresh silently moves the link to the end of the id + // order. Every read below breaks created_at_ns ties by id, and the in-memory + // backend keeps its sequence number across a refresh, so REPLACE would put the + // two backends in a different order for a re-stored row inside a tie group. SqliteStatement stmt(db_, - "INSERT OR REPLACE INTO rosbag_files " - "(fault_code, file_path, format, duration_sec, size_bytes, created_at_ns) " - "VALUES (?, ?, ?, ?, ?, ?)"); - - stmt.bind_text(1, info.fault_code); - stmt.bind_text(2, info.file_path); - stmt.bind_text(3, info.format); + "INSERT INTO rosbag_files " + "(fault_code, recording_id, file_path, format, duration_sec, size_bytes, created_at_ns) " + "VALUES (?, ?, ?, ?, ?, ?, ?) " + "ON CONFLICT(fault_code, file_path) DO UPDATE SET " + "recording_id = excluded.recording_id, format = excluded.format, " + "duration_sec = excluded.duration_sec, size_bytes = excluded.size_bytes, " + "created_at_ns = excluded.created_at_ns"); + + stmt.bind_text(1, row.fault_code); + stmt.bind_text(2, row.recording_id); + stmt.bind_text(3, row.file_path); + stmt.bind_text(4, row.format); // Bind duration_sec as a double using sqlite3_bind_double directly - if (sqlite3_bind_double(stmt.get(), 4, info.duration_sec) != SQLITE_OK) { + if (sqlite3_bind_double(stmt.get(), 5, row.duration_sec) != SQLITE_OK) { throw std::runtime_error(std::string("Failed to bind duration_sec: ") + sqlite3_errmsg(db_)); } - stmt.bind_int64(5, static_cast(info.size_bytes)); - stmt.bind_int64(6, info.created_at_ns); + stmt.bind_int64(6, static_cast(row.size_bytes)); + stmt.bind_int64(7, row.created_at_ns); if (stmt.step() != SQLITE_DONE) { throw std::runtime_error(std::string("Failed to store rosbag file: ") + sqlite3_errmsg(db_)); } - return replaced; + + if (max_rosbags_per_fault_ == 0) { + return {}; // unlimited per fault; only the global byte quota bounds this + } + + // Keep the newest N recordings of this fault. Oldest-first eviction, the same + // direction as evict_bags_over_quota, so the two eviction owners never need a + // tiebreak. At N = 1 this reproduces the pre-#620 behaviour exactly: the new + // recording replaces the old and the old bag is unlinked. + const char * const doomed_sql = + "FROM rosbag_files WHERE fault_code = ?1 AND id NOT IN " + "(SELECT id FROM rosbag_files WHERE fault_code = ?1 " + " ORDER BY created_at_ns DESC, id DESC LIMIT ?2)"; + + std::vector evicted; + { + SqliteStatement select(db_, (std::string("SELECT DISTINCT file_path ") + doomed_sql).c_str()); + select.bind_text(1, row.fault_code); + select.bind_int64(2, static_cast(max_rosbags_per_fault_)); + while (select.step() == SQLITE_ROW) { + evicted.push_back(select.column_text(0)); + } + } + if (evicted.empty()) { + return {}; + } + + SqliteStatement del(db_, (std::string("DELETE ") + doomed_sql).c_str()); + del.bind_text(1, row.fault_code); + del.bind_int64(2, static_cast(max_rosbags_per_fault_)); + if (del.step() != SQLITE_DONE) { + throw std::runtime_error(std::string("Failed to trim rosbag rows: ") + sqlite3_errmsg(db_)); + } + return evicted; +} + +namespace { + +/// Shared projection so every rosbag read decodes the same column order. +RosbagFileInfo read_rosbag_row(SqliteStatement & stmt) { + RosbagFileInfo info; + info.fault_code = stmt.column_text(0); + info.recording_id = stmt.column_text(1); + info.file_path = stmt.column_text(2); + info.format = stmt.column_text(3); + info.duration_sec = sqlite3_column_double(stmt.get(), 4); + info.size_bytes = static_cast(stmt.column_int64(5)); + info.created_at_ns = stmt.column_int64(6); + return info; +} + +constexpr const char * kRosbagColumns = + "fault_code, recording_id, file_path, format, duration_sec, size_bytes, created_at_ns"; + +} // namespace + +std::vector SqliteFaultStorage::get_rosbag_files(const std::string & fault_code) const { + std::lock_guard lock(mutex_); + + SqliteStatement stmt(db_, (std::string("SELECT ") + kRosbagColumns + + " FROM rosbag_files WHERE fault_code = ? ORDER BY created_at_ns DESC, id DESC") + .c_str()); + stmt.bind_text(1, fault_code); + + std::vector result; + while (stmt.step() == SQLITE_ROW) { + result.push_back(read_rosbag_row(stmt)); + } + return result; +} + +std::vector SqliteFaultStorage::get_rosbag_files_by_recording(const std::string & recording_id) const { + std::lock_guard lock(mutex_); + + SqliteStatement stmt(db_, (std::string("SELECT ") + kRosbagColumns + + " FROM rosbag_files WHERE recording_id = ? ORDER BY fault_code ASC") + .c_str()); + stmt.bind_text(1, recording_id); + + std::vector result; + while (stmt.step() == SQLITE_ROW) { + result.push_back(read_rosbag_row(stmt)); + } + return result; +} + +bool SqliteFaultStorage::drop_rosbag_link(const std::string & fault_code, const std::string & recording_id) { + std::lock_guard lock(mutex_); + + std::string path; + { + SqliteStatement select(db_, "SELECT file_path FROM rosbag_files WHERE fault_code = ? AND recording_id = ?"); + select.bind_text(1, fault_code); + select.bind_text(2, recording_id); + if (select.step() != SQLITE_ROW) { + return false; + } + path = select.column_text(0); + } + + exec_or_throw("BEGIN IMMEDIATE"); + try { + SqliteStatement del(db_, "DELETE FROM rosbag_files WHERE fault_code = ? AND recording_id = ?"); + del.bind_text(1, fault_code); + del.bind_text(2, recording_id); + if (del.step() != SQLITE_DONE) { + throw std::runtime_error(std::string("Failed to drop rosbag link: ") + sqlite3_errmsg(db_)); + } + exec_or_throw("COMMIT"); + } catch (...) { + sqlite3_exec(db_, "ROLLBACK", nullptr, nullptr, nullptr); + throw; + } + + if (!path_referenced(path)) { + std::error_code ec; + std::filesystem::remove_all(path, ec); + } + return true; +} + +size_t SqliteFaultStorage::delete_rosbag_recording(const std::string & recording_id) { + std::lock_guard lock(mutex_); + + std::set paths; + { + SqliteStatement select(db_, "SELECT DISTINCT file_path FROM rosbag_files WHERE recording_id = ?"); + select.bind_text(1, recording_id); + while (select.step() == SQLITE_ROW) { + paths.insert(select.column_text(0)); + } + } + if (paths.empty()) { + return 0; + } + + size_t removed = 0; + exec_or_throw("BEGIN IMMEDIATE"); + try { + SqliteStatement del(db_, "DELETE FROM rosbag_files WHERE recording_id = ?"); + del.bind_text(1, recording_id); + if (del.step() != SQLITE_DONE) { + throw std::runtime_error(std::string("Failed to delete rosbag recording: ") + sqlite3_errmsg(db_)); + } + removed = static_cast(sqlite3_changes(db_)); + exec_or_throw("COMMIT"); + } catch (...) { + sqlite3_exec(db_, "ROLLBACK", nullptr, nullptr, nullptr); + throw; + } + + for (const auto & path : paths) { + if (path_referenced(path)) { + continue; // another recording writes into the same directory - leave it + } + std::error_code ec; + std::filesystem::remove_all(path, ec); + } + return removed; } std::optional SqliteFaultStorage::get_rosbag_file(const std::string & fault_code) const { std::lock_guard lock(mutex_); + // ORDER BY is load-bearing now that a fault can hold several recordings. Without + // it SQLite may return any matching row, so the fault detail and the download + // would serve an arbitrary recording - non-deterministically, which no test + // catches reliably. id breaks the tie because a burst stamps one created_at_ns + // across all its rows. SqliteStatement stmt(db_, - "SELECT fault_code, file_path, format, duration_sec, size_bytes, created_at_ns " - "FROM rosbag_files WHERE fault_code = ?"); + "SELECT fault_code, recording_id, file_path, format, duration_sec, size_bytes, created_at_ns " + "FROM rosbag_files WHERE fault_code = ? " + "ORDER BY created_at_ns DESC, id DESC LIMIT 1"); stmt.bind_text(1, fault_code); if (stmt.step() != SQLITE_ROW) { @@ -1006,11 +1462,12 @@ std::optional SqliteFaultStorage::get_rosbag_file(const std::str RosbagFileInfo info; info.fault_code = stmt.column_text(0); - info.file_path = stmt.column_text(1); - info.format = stmt.column_text(2); - info.duration_sec = sqlite3_column_double(stmt.get(), 3); - info.size_bytes = static_cast(stmt.column_int64(4)); - info.created_at_ns = stmt.column_int64(5); + info.recording_id = stmt.column_text(1); + info.file_path = stmt.column_text(2); + info.format = stmt.column_text(3); + info.duration_sec = sqlite3_column_double(stmt.get(), 4); + info.size_bytes = static_cast(stmt.column_int64(5)); + info.created_at_ns = stmt.column_int64(6); return info; } @@ -1018,13 +1475,15 @@ std::optional SqliteFaultStorage::get_rosbag_file(const std::str bool SqliteFaultStorage::delete_rosbag_file(const std::string & fault_code) { std::lock_guard lock(mutex_); - // First get the file path so we can delete the actual file - std::string file_path; + // Every path, not the first one: a fault holds as many recordings as its cap + // allows, and stepping once would unlink one bag and leak the rest - rows gone, + // directories left behind, uncounted by a quota that sums rows. + std::set paths; { SqliteStatement select_stmt(db_, "SELECT file_path FROM rosbag_files WHERE fault_code = ?"); select_stmt.bind_text(1, fault_code); - if (select_stmt.step() == SQLITE_ROW) { - file_path = select_stmt.column_text(0); + while (select_stmt.step() == SQLITE_ROW) { + paths.insert(select_stmt.column_text(0)); } } @@ -1042,12 +1501,14 @@ bool SqliteFaultStorage::delete_rosbag_file(const std::string & fault_code) { const bool deleted = sqlite3_changes(db_) > 0; - // Unlink only once no fault references the bag any more. This row is already + // Unlink only once no fault references the bag any more. These rows are already // gone, so path_referenced() sees exactly the siblings of a shared recording. - if (!file_path.empty() && !path_referenced(file_path)) { - std::error_code ec; - std::filesystem::remove_all(file_path, ec); - // Ignore errors - file may already be deleted + for (const auto & path : paths) { + if (!path_referenced(path)) { + std::error_code ec; + std::filesystem::remove_all(path, ec); + // Ignore errors - file may already be deleted + } } return deleted; @@ -1068,9 +1529,11 @@ size_t SqliteFaultStorage::delete_rosbag_files(const std::vector & try { for (const auto & code : fault_codes) { { + // while, not if: one fault code can name several recordings now, and the + // sweep must be able to reclaim every one of their bags. SqliteStatement select_stmt(db_, "SELECT file_path FROM rosbag_files WHERE fault_code = ?"); select_stmt.bind_text(1, code); - if (select_stmt.step() == SQLITE_ROW) { + while (select_stmt.step() == SQLITE_ROW) { paths.insert(select_stmt.column_text(0)); } } @@ -1136,17 +1599,18 @@ std::vector SqliteFaultStorage::get_all_rosbag_files() const { std::vector result; SqliteStatement stmt(db_, - "SELECT fault_code, file_path, format, duration_sec, size_bytes, created_at_ns " - "FROM rosbag_files ORDER BY created_at_ns ASC"); + "SELECT fault_code, recording_id, file_path, format, duration_sec, size_bytes, created_at_ns " + "FROM rosbag_files ORDER BY created_at_ns ASC, id ASC"); while (stmt.step() == SQLITE_ROW) { RosbagFileInfo info; info.fault_code = stmt.column_text(0); - info.file_path = stmt.column_text(1); - info.format = stmt.column_text(2); - info.duration_sec = sqlite3_column_double(stmt.get(), 3); - info.size_bytes = static_cast(stmt.column_int64(4)); - info.created_at_ns = stmt.column_int64(5); + info.recording_id = stmt.column_text(1); + info.file_path = stmt.column_text(2); + info.format = stmt.column_text(3); + info.duration_sec = sqlite3_column_double(stmt.get(), 4); + info.size_bytes = static_cast(stmt.column_int64(5)); + info.created_at_ns = stmt.column_int64(6); result.push_back(info); } @@ -1162,22 +1626,24 @@ std::vector SqliteFaultStorage::list_rosbags_for_entity(const st // Use json_each() for proper JSON array querying instead of LIKE, which treats // '_' as a single-char wildcard and would produce false positives on ROS names. SqliteStatement stmt(db_, - "SELECT r.fault_code, r.file_path, r.format, r.duration_sec, r.size_bytes, " + "SELECT r.fault_code, r.recording_id, r.file_path, r.format, r.duration_sec, r.size_bytes, " "r.created_at_ns " "FROM rosbag_files r " "JOIN faults f ON r.fault_code = f.fault_code " - "JOIN json_each(f.reporting_sources) j ON j.value = ?"); + "JOIN json_each(f.reporting_sources) j ON j.value = ? " + "ORDER BY r.created_at_ns DESC, r.id DESC"); stmt.bind_text(1, entity_fqn); while (stmt.step() == SQLITE_ROW) { RosbagFileInfo info; info.fault_code = stmt.column_text(0); - info.file_path = stmt.column_text(1); - info.format = stmt.column_text(2); - info.duration_sec = sqlite3_column_double(stmt.get(), 3); - info.size_bytes = static_cast(stmt.column_int64(4)); - info.created_at_ns = stmt.column_int64(5); + info.recording_id = stmt.column_text(1); + info.file_path = stmt.column_text(2); + info.format = stmt.column_text(3); + info.duration_sec = sqlite3_column_double(stmt.get(), 4); + info.size_bytes = static_cast(stmt.column_int64(5)); + info.created_at_ns = stmt.column_int64(6); result.push_back(info); } 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 8fb8928e8..bc20e69a0 100644 --- a/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp +++ b/src/ros2_medkit_fault_manager/test/test_fault_manager.cpp @@ -1520,25 +1520,79 @@ TEST(MatchesEntityTest, EmptySources) { // --- InMemoryFaultStorage snapshot limit tests --- -TEST(InMemorySnapshotLimitTest, RejectsWhenFull) { +TEST(InMemorySnapshotRetentionTest, ClearKeepsSnapshotsWhenEvidenceIsRetained) { InMemoryFaultStorage storage; - storage.set_max_snapshots_per_fault(2); + rclcpp::Clock clock; + storage.report_fault_event("KEEP", ros2_medkit_msgs::srv::ReportFault::Request::EVENT_FAILED, + ros2_medkit_msgs::msg::Fault::SEVERITY_ERROR, "keep", "/n", clock.now(), + ros2_medkit_fault_manager::DebounceConfig{}); + storage.set_retain_snapshots_on_clear(true); - ros2_medkit_fault_manager::SnapshotData snap; - snap.fault_code = "TEST"; - snap.topic = "/test"; - snap.message_type = "std_msgs/msg/String"; - snap.data = "{}"; + ros2_medkit_fault_manager::SnapshotData row; + row.fault_code = "KEEP"; + row.topic = "/t"; + row.message_type = "std_msgs/msg/String"; + row.data = "{}"; + row.captured_at_ns = 1000; + row.capture_id = 1; + storage.store_snapshots({row}); - snap.captured_at_ns = 1000; - storage.store_snapshot(snap); - snap.captured_at_ns = 2000; - storage.store_snapshot(snap); - snap.captured_at_ns = 3000; - storage.store_snapshot(snap); // Should be rejected + ASSERT_TRUE(storage.clear_fault("KEEP")); + // Same rule as the SQLite side: with a history configured, acknowledging must + // not leave recordings whose matching readings were deleted. + EXPECT_EQ(storage.get_snapshots("KEEP").size(), 1u); +} + +TEST(InMemorySnapshotRetentionTest, ClearStillDropsSnapshotsByDefault) { + InMemoryFaultStorage storage; + rclcpp::Clock clock; + storage.report_fault_event("DROP", ros2_medkit_msgs::srv::ReportFault::Request::EVENT_FAILED, + ros2_medkit_msgs::msg::Fault::SEVERITY_ERROR, "drop", "/n", clock.now(), + ros2_medkit_fault_manager::DebounceConfig{}); + + ros2_medkit_fault_manager::SnapshotData row; + row.fault_code = "DROP"; + row.topic = "/t"; + row.message_type = "std_msgs/msg/String"; + row.data = "{}"; + row.capture_id = 1; + storage.store_snapshots({row}); + + ASSERT_TRUE(storage.clear_fault("DROP")); + EXPECT_TRUE(storage.get_snapshots("DROP").empty()) << "the default must stay what it always was"; +} + +TEST(InMemorySnapshotLimitTest, KeepsTheNewestCaptureWholeAndDropsTheOldest) { + InMemoryFaultStorage storage; + storage.set_max_snapshots_per_fault(4); + + const auto capture = [](int64_t id, int64_t at) { + std::vector rows; + for (const char * topic : {"/a", "/b"}) { + ros2_medkit_fault_manager::SnapshotData row; + row.fault_code = "TEST"; + row.topic = topic; + row.message_type = "std_msgs/msg/String"; + row.data = "{}"; + row.captured_at_ns = at; + row.capture_id = id; + rows.push_back(row); + } + return rows; + }; + + storage.store_snapshots(capture(1, 1000)); + storage.store_snapshots(capture(2, 2000)); + storage.store_snapshots(capture(3, 3000)); + + // Whole captures in, whole capture out. Counting rows and rejecting the new one + // stored capture 3 in part: one topic present, the rest silently absent. auto result = storage.get_snapshots("TEST"); - EXPECT_EQ(result.size(), 2u); + ASSERT_EQ(result.size(), 4u); + for (const auto & s : result) { + EXPECT_NE(s.capture_id, 1) << "the oldest capture should have gone whole"; + } } TEST(InMemorySnapshotLimitTest, UnlimitedWhenZero) { diff --git a/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp b/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp index 5c79fa5a6..e0048b81a 100644 --- a/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp +++ b/src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp @@ -935,7 +935,7 @@ class RosbagMetadataFailingStorage : public InMemoryFaultStorage { /// are durable and the bag they name must survive the failure. class RosbagQuotaSweepFailingStorage : public InMemoryFaultStorage { public: - size_t delete_rosbag_files(const std::vector & /*fault_codes*/) override { + size_t delete_rosbag_recording(const std::string & /*recording_id*/) override { ++sweep_attempts; throw std::runtime_error("quota sweep unavailable"); } @@ -1104,6 +1104,62 @@ TEST_F(RosbagCaptureIntegrationTest, FullFaultLifecycleWithNoMessages) { EXPECT_FALSE(capture.is_running()); } +TEST_F(RosbagCaptureIntegrationTest, AcknowledgingKeepsAHistoryTheOperatorConfiguredToKeep) { + // auto_cleanup deletes EVERY recording of the fault, so with a cap above one the + // first acknowledgement wiped the whole trail max_bags_per_fault had just been + // raised to collect - one default silently cancelling another. Raising the cap is + // an explicit request to keep a history, so retention governs and acknowledgement + // leaves the evidence alone. + InMemoryFaultStorage storage; + auto rosbag_config = create_rosbag_config(); + rosbag_config.duration_sec = 1.0; + rosbag_config.duration_after_sec = 0.0; + rosbag_config.auto_cleanup = true; + rosbag_config.max_bags_per_fault = 3; + storage.set_max_rosbags_per_fault(rosbag_config.max_bags_per_fault); + + auto snapshot_config = create_snapshot_config(); + RosbagCapture capture(node_.get(), &storage, rosbag_config, snapshot_config); + capture.start(); + fill_buffer("/keep_history_probe"); + capture.on_fault_confirmed("KEEPS_HISTORY"); + spin_for(std::chrono::milliseconds(300)); + + ASSERT_FALSE(storage.get_rosbag_files("KEEPS_HISTORY").empty()) << "nothing was recorded to keep"; + const auto before = storage.get_rosbag_files("KEEPS_HISTORY").size(); + + capture.on_fault_cleared("KEEPS_HISTORY"); + + EXPECT_EQ(storage.get_rosbag_files("KEEPS_HISTORY").size(), before) + << "acknowledging the fault destroyed the recordings the cap was raised to keep"; + capture.stop(); +} + +TEST_F(RosbagCaptureIntegrationTest, AcknowledgingStillCleansUpWhenNoHistoryIsConfigured) { + // The shipped default: one recording per fault means there is no history to + // protect, so auto_cleanup keeps behaving exactly as it always has. + InMemoryFaultStorage storage; + auto rosbag_config = create_rosbag_config(); + rosbag_config.duration_sec = 1.0; + rosbag_config.duration_after_sec = 0.0; + rosbag_config.auto_cleanup = true; + rosbag_config.max_bags_per_fault = 1; + + auto snapshot_config = create_snapshot_config(); + RosbagCapture capture(node_.get(), &storage, rosbag_config, snapshot_config); + capture.start(); + fill_buffer("/cleanup_probe"); + capture.on_fault_confirmed("CLEANED_UP"); + spin_for(std::chrono::milliseconds(300)); + + ASSERT_FALSE(storage.get_rosbag_files("CLEANED_UP").empty()); + + capture.on_fault_cleared("CLEANED_UP"); + + EXPECT_TRUE(storage.get_rosbag_files("CLEANED_UP").empty()) << "auto_cleanup stopped working at the default cap"; + capture.stop(); +} + TEST_F(RosbagCaptureIntegrationTest, MultipleFaultsHandled) { auto rosbag_config = create_rosbag_config(); rosbag_config.duration_sec = 0.5; diff --git a/src/ros2_medkit_fault_manager/test/test_rosbag_history.test.py b/src/ros2_medkit_fault_manager/test/test_rosbag_history.test.py new file mode 100644 index 000000000..06175d845 --- /dev/null +++ b/src/ros2_medkit_fault_manager/test/test_rosbag_history.test.py @@ -0,0 +1,477 @@ +#!/usr/bin/env python3 +# Copyright 2026 mfaferek93 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +""" +End-to-end tests for keeping several recordings per fault code (Issue #620). + +The other rosbag suites all run at the shipped default of one recording per +fault, which is what every release before this one could store. This one raises +the cap and drives the case the contributor reported: a fault that confirms, +clears and confirms again. Under the old schema the second recording silently +replaced the first, so the black box only ever held the most recent event. + +Runs against SQLite rather than the in-memory backend on purpose. SQLite is the +shipped default, it enforces the cap in SQL, and the unique index this change +introduces only exists there - the in-memory parity is covered by +test_rosbag_storage_parity at unit level. +""" + +import os +import tempfile +import time +import unittest + +from launch import LaunchDescription +import launch.actions +import launch_ros.actions +import launch_testing.actions +import launch_testing.markers +import rclpy +from rclpy.node import Node +from rclpy.qos import HistoryPolicy, QoSProfile, ReliabilityPolicy +from ros2_medkit_msgs.msg import Fault, FaultEvent +from ros2_medkit_msgs.srv import ClearFault, GetRosbag, ListRosbags, ReportFault +from sensor_msgs.msg import Temperature + + +def get_coverage_env(): + """Get environment variables for gcov coverage data collection.""" + try: + from ament_index_python.packages import get_package_prefix + pkg_prefix = get_package_prefix('ros2_medkit_fault_manager') + workspace = os.path.dirname(os.path.dirname(pkg_prefix)) + build_dir = os.path.join(workspace, 'build', 'ros2_medkit_fault_manager') + + if os.path.exists(build_dir): + return { + 'GCOV_PREFIX': build_dir, + 'GCOV_PREFIX_STRIP': str(build_dir.count(os.sep)), + } + except Exception: + # Coverage environment is optional; on any error, fall back to no extra coverage config + pass + return {} + + +ROSBAG_STORAGE_PATH = tempfile.mkdtemp(prefix='rosbag_history_') +DATABASE_PATH = os.path.join(ROSBAG_STORAGE_PATH, 'faults.db') +MAX_BAGS_PER_FAULT = 3 + +PUBLISHER_SCRIPT_PATH = None + + +def generate_test_description(): + """Launch a fault_manager keeping several recordings per fault code.""" + publisher_script = """ +import rclpy +from rclpy.node import Node +from rclpy.qos import QoSProfile, ReliabilityPolicy, HistoryPolicy +from sensor_msgs.msg import Temperature +from std_msgs.msg import String + +class TestPublisher(Node): + def __init__(self): + super().__init__('history_publisher') + qos = QoSProfile( + reliability=ReliabilityPolicy.BEST_EFFORT, + history=HistoryPolicy.KEEP_LAST, + depth=10 + ) + self.temp_pub = self.create_publisher(Temperature, '/test/temperature', qos) + self.string_pub = self.create_publisher(String, '/test/status', qos) + self.timer = self.create_timer(0.1, self.publish) + self.counter = 0 + + def publish(self): + temp_msg = Temperature() + temp_msg.temperature = 25.0 + self.counter * 0.1 + temp_msg.variance = 0.1 + self.temp_pub.publish(temp_msg) + + string_msg = String() + string_msg.data = f'status_{self.counter}' + self.string_pub.publish(string_msg) + self.counter += 1 + +def main(): + rclpy.init() + node = TestPublisher() + try: + rclpy.spin(node) + except KeyboardInterrupt: + pass + finally: + node.destroy_node() + rclpy.shutdown() + +if __name__ == '__main__': + main() +""" + + global PUBLISHER_SCRIPT_PATH + script_file = tempfile.NamedTemporaryFile(mode='w', suffix='.py', delete=False) + script_file.write(publisher_script) + script_file.close() + PUBLISHER_SCRIPT_PATH = script_file.name + + env = os.environ.copy() + env['ROS_LOCALHOST_ONLY'] = '1' + + test_publisher = launch.actions.ExecuteProcess( + cmd=['python3', script_file.name], + output='screen', + name='history_publisher', + env=env, + ) + + fault_manager_env = get_coverage_env() + fault_manager_env['ROS_LOCALHOST_ONLY'] = '1' + + fault_manager_node = launch_ros.actions.Node( + package='ros2_medkit_fault_manager', + executable='fault_manager_node', + name='fault_manager', + output='screen', + additional_env=fault_manager_env, + parameters=[{ + # SQLite, not memory: the cap is enforced in SQL there, and the + # unique index that replaced the old UNIQUE(fault_code) only exists + # in this backend. + 'storage_type': 'sqlite', + 'database_path': DATABASE_PATH, + 'confirmation_threshold': -1, + 'snapshots.rosbag.enabled': True, + 'snapshots.rosbag.duration_sec': 2.0, + 'snapshots.rosbag.duration_after_sec': 0.5, + 'snapshots.rosbag.topics': '/test/temperature,/test/status', + 'snapshots.rosbag.format': 'mcap', + 'snapshots.rosbag.storage_path': ROSBAG_STORAGE_PATH, + 'snapshots.rosbag.max_bag_size_mb': 10, + 'snapshots.rosbag.max_total_storage_mb': 50, + # The whole point: keep a history instead of replacing. + 'snapshots.rosbag.max_bags_per_fault': MAX_BAGS_PER_FAULT, + # Clearing a fault must not take its recordings with it - the + # scenario under test is confirm / clear / confirm. + 'snapshots.rosbag.auto_cleanup': False, + 'snapshots.rosbag.lazy_start': False, + }], + sigterm_timeout='30', + sigkill_timeout='15', + ) + + delayed_fault_manager = launch.actions.TimerAction( + period=8.0, + actions=[fault_manager_node], + ) + + return ( + LaunchDescription([ + test_publisher, + delayed_fault_manager, + launch_testing.actions.ReadyToTest(), + ]), + { + 'fault_manager_node': fault_manager_node, + 'test_publisher': test_publisher, + }, + ) + + +class TestRosbagHistory(unittest.TestCase): + """Several recordings per fault code, addressed by recording id.""" + + @classmethod + def setUpClass(cls): + os.environ['ROS_LOCALHOST_ONLY'] = '1' + rclpy.init() + cls.node = Node('test_rosbag_history_client') + + cls.report_fault_client = cls.node.create_client( + ReportFault, '/fault_manager/report_fault' + ) + cls.clear_fault_client = cls.node.create_client( + ClearFault, '/fault_manager/clear_fault' + ) + cls.get_rosbag_client = cls.node.create_client( + GetRosbag, '/fault_manager/get_rosbag' + ) + cls.list_rosbags_client = cls.node.create_client( + ListRosbags, '/fault_manager/list_rosbags' + ) + + assert cls.report_fault_client.wait_for_service(timeout_sec=15.0), \ + 'report_fault service not available' + assert cls.clear_fault_client.wait_for_service(timeout_sec=15.0), \ + 'clear_fault service not available' + assert cls.get_rosbag_client.wait_for_service(timeout_sec=15.0), \ + 'get_rosbag service not available' + assert cls.list_rosbags_client.wait_for_service(timeout_sec=15.0), \ + 'list_rosbags service not available' + + deadline = time.time() + 15.0 + while (not cls.node.get_publishers_info_by_topic('/test/temperature') + and time.time() < deadline): + time.sleep(0.2) + time.sleep(3.0) + + @classmethod + def tearDownClass(cls): + cls.node.destroy_node() + rclpy.shutdown() + + def _call_service(self, client, request, timeout_sec=10.0): + future = client.call_async(request) + rclpy.spin_until_future_complete(self.node, future, timeout_sec=timeout_sec) + self.assertIsNotNone(future.result(), 'Service call timed out') + return future.result() + + def _report_fault(self, fault_code, description='History test fault'): + request = ReportFault.Request() + request.fault_code = fault_code + request.event_type = ReportFault.Request.EVENT_FAILED + request.severity = Fault.SEVERITY_ERROR + request.description = description + request.source_id = '/test_node' + return self._call_service(self.report_fault_client, request) + + def _clear_fault(self, fault_code): + request = ClearFault.Request() + request.fault_code = fault_code + return self._call_service(self.clear_fault_client, request) + + def _get_by_fault(self, fault_code, timeout=12.0): + """Poll GetRosbag by fault code until the post-roll recording lands.""" + request = GetRosbag.Request() + request.fault_code = fault_code + deadline = time.time() + timeout + response = self._call_service(self.get_rosbag_client, request) + while time.time() < deadline: + if response is not None and response.success: + return response + time.sleep(0.5) + response = self._call_service(self.get_rosbag_client, request) + return response + + def _wait_for_new_recording(self, fault_code, known_ids, timeout=15.0): + """Poll until the fault's newest recording is one we have not seen.""" + deadline = time.time() + timeout + response = None + while time.time() < deadline: + response = self._get_by_fault(fault_code, timeout=2.0) + if (response is not None and response.success + and response.recording_id not in known_ids): + return response + time.sleep(0.5) + return response + + def _get_by_recording(self, recording_id): + request = GetRosbag.Request() + request.recording_id = recording_id + return self._call_service(self.get_rosbag_client, request) + + def _wait_for_buffered_data(self, count=5, timeout=8.0): + """ + Wait until the ring buffer holds fresh data before a confirmation. + + A previous fault's post-roll diverts incoming messages straight into the + open bag, so right after that window closes the buffer can be empty and + the next confirmation produces no bag at all. Reset the streak on every + CONFIRMED so the messages counted here provably arrived with no post-roll + newly opened partway through - see the same helper in + test_rosbag_integration. + """ + received = 0 + + def _cb(_msg): + nonlocal received + received += 1 + + def _on_event(msg): + nonlocal received + if msg.event_type == FaultEvent.EVENT_CONFIRMED: + received = 0 + + qos = QoSProfile( + reliability=ReliabilityPolicy.BEST_EFFORT, + history=HistoryPolicy.KEEP_LAST, + depth=10, + ) + events_qos = QoSProfile( + reliability=ReliabilityPolicy.RELIABLE, + history=HistoryPolicy.KEEP_LAST, + depth=100, + ) + sub = self.node.create_subscription(Temperature, '/test/temperature', _cb, qos) + events_sub = self.node.create_subscription( + FaultEvent, '/fault_manager/events', _on_event, events_qos + ) + try: + deadline = time.time() + timeout + while received < count and time.time() < deadline: + rclpy.spin_once(self.node, timeout_sec=0.1) + finally: + self.node.destroy_subscription(sub) + self.node.destroy_subscription(events_sub) + return received >= count + + def _record_occurrence(self, fault_code, known_ids): + """Confirm the fault once and return its new recording.""" + self.assertTrue(self._wait_for_buffered_data(), + 'ring buffer never refilled after the previous post-roll') + response = self._report_fault(fault_code) + self.assertTrue(response.accepted) + + recording = self._wait_for_new_recording(fault_code, known_ids) + self.assertIsNotNone(recording) + self.assertTrue(recording.success, + f'no recording for {fault_code}: {recording.error_message}') + self.assertNotIn(recording.recording_id, known_ids, + 'the re-confirmation reused the previous recording') + return recording + + def test_01_reconfirming_a_fault_keeps_the_earlier_recording(self): + """The reported bug: the second confirmation used to overwrite the first.""" + fault_code = 'FLAPPING_SENSOR' + + first = self._record_occurrence(fault_code, set()) + self.assertTrue(os.path.exists(first.file_path)) + print(f'First occurrence: {first.recording_id}') + + # Clearing is what a technician does after acknowledging. With + # auto_cleanup off the evidence has to survive it. + self.assertTrue(self._clear_fault(fault_code).success) + + second = self._record_occurrence(fault_code, {first.recording_id}) + print(f'Second occurrence: {second.recording_id}') + + self.assertNotEqual(first.file_path, second.file_path, + 'the two occurrences must be separate bags') + self.assertTrue(os.path.exists(first.file_path), + 'the first recording was overwritten - this is issue #620') + self.assertTrue(os.path.exists(second.file_path)) + + # Both remain individually addressable by their own id. + for recording in (first, second): + fetched = self._get_by_recording(recording.recording_id) + self.assertTrue(fetched.success, + f'recording {recording.recording_id} not retrievable: ' + f'{fetched.error_message}') + self.assertEqual(fetched.file_path, recording.file_path) + self.assertIn(fault_code, fetched.fault_codes) + + def test_02_fault_code_lookup_still_serves_the_newest_recording(self): + """The compatibility window: a fault-code URL keeps working.""" + fault_code = 'FLAPPING_SENSOR' + + # Established by test_01; the newest of them is what a bare fault code + # has always meant, and must still mean. + newest = self._get_by_fault(fault_code) + self.assertTrue(newest.success) + self.assertGreater(len(newest.recording_id), 0, + 'a fault-code lookup must still name the recording it served') + + by_id = self._get_by_recording(newest.recording_id) + self.assertTrue(by_id.success) + self.assertEqual(by_id.file_path, newest.file_path, + 'both addressing modes must resolve to the same bytes') + self.assertEqual(by_id.recording_id, newest.recording_id) + + def test_03_the_cap_evicts_the_oldest_recording(self): + """Past max_bags_per_fault the oldest goes, newest are kept.""" + fault_code = 'CAPPED_FAULT' + + recordings = [] + seen = set() + for _ in range(MAX_BAGS_PER_FAULT + 1): + self.assertTrue(self._clear_fault(fault_code).success or True) + recording = self._record_occurrence(fault_code, seen) + seen.add(recording.recording_id) + recordings.append(recording) + + oldest = recordings[0] + survivors = recordings[1:] + + # The eviction happens inside the store, so it is done by the time the + # newest recording is readable. + deadline = time.time() + 10.0 + while os.path.exists(oldest.file_path) and time.time() < deadline: + time.sleep(0.5) + + self.assertFalse(os.path.exists(oldest.file_path), + f'the cap of {MAX_BAGS_PER_FAULT} did not evict the oldest bag') + evicted = self._get_by_recording(oldest.recording_id) + self.assertFalse(evicted.success, 'the evicted recording is still addressable') + + for recording in survivors: + self.assertTrue(os.path.exists(recording.file_path), + f'{recording.recording_id} was evicted but is within the cap') + self.assertTrue(self._get_by_recording(recording.recording_id).success) + + def test_04_list_rosbags_names_every_recording(self): + """The gateway builds one descriptor per recording out of this list.""" + request = ListRosbags.Request() + request.entity_fqn = '/test_node' + response = self._call_service(self.list_rosbags_client, request) + + self.assertTrue(response.success, response.error_message) + self.assertEqual(len(response.recording_ids), len(response.file_paths), + 'recording_ids must be parallel to the other arrays') + self.assertEqual(len(response.recording_ids), len(response.fault_codes)) + + # A fault that confirmed several times contributes several rows, each + # naming its own recording. Before this change it could only ever + # contribute one. + capped_ids = {rid for rid, code in zip(response.recording_ids, response.fault_codes) + if code == 'CAPPED_FAULT'} + self.assertEqual(len(capped_ids), MAX_BAGS_PER_FAULT, + f'expected {MAX_BAGS_PER_FAULT} recordings for CAPPED_FAULT, ' + f'got {sorted(capped_ids)}') + + for recording_id, file_path in zip(response.recording_ids, response.file_paths): + self.assertEqual(recording_id, os.path.basename(file_path.rstrip('/')), + 'recording id must be the bag directory basename') + + def test_05_unknown_recording_id_is_reported_not_guessed(self): + """An id that is neither a recording nor a fault code fails cleanly.""" + response = self._get_by_recording('fault_NO_SUCH_BAG_1700000000000') + self.assertFalse(response.success) + self.assertGreater(len(response.error_message), 0) + + def test_06_a_traversal_shaped_recording_id_is_rejected(self): + """The id becomes a URL segment and is validated like a fault code.""" + for bad_id in ('../../etc/passwd', 'fault/../../X', 'has spaces'): + response = self._get_by_recording(bad_id) + self.assertFalse(response.success, f'{bad_id!r} was accepted') + + +@launch_testing.post_shutdown_test() +class TestRosbagHistoryShutdown(unittest.TestCase): + """Post-shutdown tests.""" + + def test_exit_code(self, proc_info): + """Verify fault_manager exits cleanly.""" + launch_testing.asserts.assertExitCodes( + proc_info, + process='fault_manager_node' + ) + + def test_cleanup_temp_directory(self): + """Clean up temporary rosbag storage directory and publisher script.""" + import shutil + if os.path.exists(ROSBAG_STORAGE_PATH): + shutil.rmtree(ROSBAG_STORAGE_PATH, ignore_errors=True) + + if PUBLISHER_SCRIPT_PATH and os.path.exists(PUBLISHER_SCRIPT_PATH): + os.unlink(PUBLISHER_SCRIPT_PATH) diff --git a/src/ros2_medkit_fault_manager/test/test_rosbag_storage_parity.cpp b/src/ros2_medkit_fault_manager/test/test_rosbag_storage_parity.cpp new file mode 100644 index 000000000..9c665464a --- /dev/null +++ b/src/ros2_medkit_fault_manager/test/test_rosbag_storage_parity.cpp @@ -0,0 +1,414 @@ +// Copyright 2026 mfaferek93 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/// Rosbag retention parity between the two storage backends. +/// +/// A fault holding several recordings was enforced by the SQLite schema before, and +/// separately by a std::map in the in-memory backend. Two enforcement points means two +/// chances to diverge, and `storage_type: memory` silently keeping the old behaviour +/// would be invisible to a suite that only exercises SQLite. Every assertion here +/// therefore runs against both backends from one body. + +#include + +#include +#include +#include +#include +#include +#include +#include + +#include "ros2_medkit_fault_manager/fault_storage.hpp" +#include "ros2_medkit_fault_manager/sqlite_fault_storage.hpp" + +namespace { + +using ros2_medkit_fault_manager::FaultStorage; +using ros2_medkit_fault_manager::InMemoryFaultStorage; +using ros2_medkit_fault_manager::rosbag_recording_id; +using ros2_medkit_fault_manager::RosbagFileInfo; +using ros2_medkit_fault_manager::SqliteFaultStorage; + +/// Backends differ only in how they are constructed, so the fixture reaches them +/// through this rather than through #ifdef-style branching inside the test bodies. +template +struct BackendFactory; + +template <> +struct BackendFactory { + static std::unique_ptr make(const std::filesystem::path & /*dir*/) { + return std::make_unique(); + } +}; + +template <> +struct BackendFactory { + static std::unique_ptr make(const std::filesystem::path & dir) { + return std::make_unique((dir / "parity.db").string()); + } +}; + +template +class RosbagRetentionParityTest : public ::testing::Test { + protected: + void SetUp() override { + dir_ = + std::filesystem::temp_directory_path() / ("medkit_parity_" + std::to_string(reinterpret_cast(this))); + std::filesystem::remove_all(dir_); + std::filesystem::create_directories(dir_); + storage_ = BackendFactory::make(dir_); + } + + void TearDown() override { + storage_.reset(); + std::error_code ec; + std::filesystem::remove_all(dir_, ec); + } + + /// Create a real bag directory so unlink behaviour is observable, not just row state. + /// Bags are directories on disk, which is what both backends remove. + std::string make_bag(const std::string & name) { + const auto path = dir_ / name; + std::filesystem::create_directories(path); + std::ofstream(path / "metadata.yaml") << "version: 9\n"; + return path.string(); + } + + RosbagFileInfo row(const std::string & code, const std::string & bag_name, int64_t created_ns) { + RosbagFileInfo info; + info.fault_code = code; + info.file_path = make_bag(bag_name); + info.recording_id = rosbag_recording_id(info.file_path); + info.format = "mcap"; + info.duration_sec = 5.0; + info.size_bytes = 1024; + info.created_at_ns = created_ns; + return info; + } + + std::filesystem::path dir_; + std::unique_ptr storage_; +}; + +using Backends = ::testing::Types; +TYPED_TEST_SUITE(RosbagRetentionParityTest, Backends); + +TYPED_TEST(RosbagRetentionParityTest, RecordingIdIsTheBasenameOfEveryStoredRow) { + // The gateway addresses a recording by this string and the fault_manager resolves it + // back to rows, so a backend that stored anything else would break the URL silently. + this->storage_->set_max_rosbags_per_fault(0); + this->storage_->store_rosbag_file(this->row("BASENAME", "fault_BASENAME_1", 1000)); + this->storage_->store_rosbag_file(this->row("BASENAME", "fault_BASENAME_2", 2000)); + + const auto rows = this->storage_->get_rosbag_files("BASENAME"); + ASSERT_EQ(rows.size(), 2u); + for (const auto & r : rows) { + EXPECT_EQ(r.recording_id, std::filesystem::path(r.file_path).filename().string()); + } +} + +TYPED_TEST(RosbagRetentionParityTest, RecordingIdIsDerivedWhenTheCallerLeftItEmpty) { + // rosbag_capture fills it, but the interface is public and embedders re-store rows. + // A row with no recording_id is unaddressable, so neither backend may store one. + this->storage_->set_max_rosbags_per_fault(0); + auto info = this->row("DERIVED", "fault_DERIVED_1", 1000); + info.recording_id.clear(); + this->storage_->store_rosbag_file(info); + + const auto rows = this->storage_->get_rosbag_files("DERIVED"); + ASSERT_EQ(rows.size(), 1u); + EXPECT_EQ(rows[0].recording_id, "fault_DERIVED_1"); +} + +TYPED_TEST(RosbagRetentionParityTest, SeveralRecordingsForOneFaultSurviveUnderACap) { + // The feature itself: this is exactly what the schema made impossible before. + this->storage_->set_max_rosbags_per_fault(3); + this->storage_->store_rosbag_file(this->row("FLAP", "fault_FLAP_1", 1000)); + this->storage_->store_rosbag_file(this->row("FLAP", "fault_FLAP_2", 2000)); + this->storage_->store_rosbag_file(this->row("FLAP", "fault_FLAP_3", 3000)); + + const auto rows = this->storage_->get_rosbag_files("FLAP"); + ASSERT_EQ(rows.size(), 3u); + EXPECT_EQ(rows[0].recording_id, "fault_FLAP_3") << "newest first"; + EXPECT_EQ(rows[2].recording_id, "fault_FLAP_1"); + for (const auto & r : rows) { + EXPECT_TRUE(std::filesystem::exists(r.file_path)); + } +} + +TYPED_TEST(RosbagRetentionParityTest, CapKeepsTheNewestAndUnlinksTheEvicted) { + this->storage_->set_max_rosbags_per_fault(2); + const auto oldest = this->row("CAP", "fault_CAP_1", 1000); + this->storage_->store_rosbag_file(oldest); + this->storage_->store_rosbag_file(this->row("CAP", "fault_CAP_2", 2000)); + this->storage_->store_rosbag_file(this->row("CAP", "fault_CAP_3", 3000)); + + const auto rows = this->storage_->get_rosbag_files("CAP"); + ASSERT_EQ(rows.size(), 2u); + EXPECT_EQ(rows[0].recording_id, "fault_CAP_3"); + EXPECT_EQ(rows[1].recording_id, "fault_CAP_2"); + EXPECT_FALSE(std::filesystem::exists(oldest.file_path)) << "the evicted bag must not outlive its last row"; +} + +TYPED_TEST(RosbagRetentionParityTest, ACapOfOneReproducesThePreviousBehaviourExactly) { + // The shipped default. Landing at parity is what makes any regression report about + // the plumbing rather than about the retention policy. + this->storage_->set_max_rosbags_per_fault(1); + const auto first = this->row("ONE", "fault_ONE_1", 1000); + this->storage_->store_rosbag_file(first); + this->storage_->store_rosbag_file(this->row("ONE", "fault_ONE_2", 2000)); + + const auto rows = this->storage_->get_rosbag_files("ONE"); + ASSERT_EQ(rows.size(), 1u); + EXPECT_EQ(rows[0].recording_id, "fault_ONE_2"); + EXPECT_FALSE(std::filesystem::exists(first.file_path)); + + const auto newest = this->storage_->get_rosbag_file("ONE"); + ASSERT_TRUE(newest.has_value()); + EXPECT_EQ(newest->recording_id, "fault_ONE_2"); +} + +TYPED_TEST(RosbagRetentionParityTest, ACapOfOneIsTheDefaultWithoutConfiguringAnything) { + // A backend constructed directly - tests, embedders - must not silently start + // accumulating recordings just because the cap became configurable. + const auto first = this->row("DEFAULT", "fault_DEFAULT_1", 1000); + this->storage_->store_rosbag_file(first); + this->storage_->store_rosbag_file(this->row("DEFAULT", "fault_DEFAULT_2", 2000)); + + EXPECT_EQ(this->storage_->get_rosbag_files("DEFAULT").size(), 1u); + EXPECT_FALSE(std::filesystem::exists(first.file_path)); +} + +TYPED_TEST(RosbagRetentionParityTest, ZeroMeansUnlimited) { + this->storage_->set_max_rosbags_per_fault(0); + for (int i = 1; i <= 6; ++i) { + this->storage_->store_rosbag_file(this->row("UNBOUNDED", "fault_UNBOUNDED_" + std::to_string(i), i * 1000)); + } + EXPECT_EQ(this->storage_->get_rosbag_files("UNBOUNDED").size(), 6u); +} + +TYPED_TEST(RosbagRetentionParityTest, ReStoringTheSamePathIsAnUpsertNotASecondRecording) { + // Uniqueness is on (fault_code, file_path). Re-storing the same bag - a restart + // replaying its rows, say - must update in place, not consume a cap slot. + this->storage_->set_max_rosbags_per_fault(3); + auto info = this->row("UPSERT", "fault_UPSERT_1", 1000); + this->storage_->store_rosbag_file(info); + info.size_bytes = 4096; + info.duration_sec = 9.0; + this->storage_->store_rosbag_file(info); + + const auto rows = this->storage_->get_rosbag_files("UPSERT"); + ASSERT_EQ(rows.size(), 1u); + EXPECT_EQ(rows[0].size_bytes, 4096u); + EXPECT_DOUBLE_EQ(rows[0].duration_sec, 9.0); + EXPECT_TRUE(std::filesystem::exists(rows[0].file_path)) << "an upsert must not unlink the bag it just updated"; +} + +TYPED_TEST(RosbagRetentionParityTest, RefreshingALinkDoesNotMoveItWithinATieGroup) { + // The tiebreak is positional - SQLite's id, the in-memory sequence number - so a + // refresh must not renumber the row. SQLite's INSERT OR REPLACE would: it deletes + // and re-inserts, handing the row a fresh id and silently promoting it past rows + // it was stored before. + this->storage_->set_max_rosbags_per_fault(0); + auto first = this->row("REFRESH", "fault_REFRESH_1", 5000); + this->storage_->store_rosbag_file(first); + this->storage_->store_rosbag_file(this->row("REFRESH", "fault_REFRESH_2", 5000)); + + first.size_bytes = 8192; + this->storage_->store_rosbag_file(first); + + const auto rows = this->storage_->get_rosbag_files("REFRESH"); + ASSERT_EQ(rows.size(), 2u); + EXPECT_EQ(rows[0].recording_id, "fault_REFRESH_2") << "the refresh must not reorder the tie group"; + EXPECT_EQ(rows[1].recording_id, "fault_REFRESH_1"); + EXPECT_EQ(rows[1].size_bytes, 8192u) << "but it must still update the row"; +} + +TYPED_TEST(RosbagRetentionParityTest, NewestFirstIsStableWhenAWholeBurstSharesATimestamp) { + // Every row of one burst carries the same created_at_ns, so ties are guaranteed + // rather than exotic. SQLite breaks them by id; the in-memory backend needs its own + // sequence counter to agree. Without it the two orders diverge only sometimes, + // which is a flaky test rather than an honest failure. + this->storage_->set_max_rosbags_per_fault(0); + for (int i = 1; i <= 4; ++i) { + this->storage_->store_rosbag_file(this->row("TIE", "fault_TIE_" + std::to_string(i), 7000)); + } + + const auto rows = this->storage_->get_rosbag_files("TIE"); + ASSERT_EQ(rows.size(), 4u); + EXPECT_EQ(rows[0].recording_id, "fault_TIE_4") << "insertion order breaks the tie, newest first"; + EXPECT_EQ(rows[1].recording_id, "fault_TIE_3"); + EXPECT_EQ(rows[2].recording_id, "fault_TIE_2"); + EXPECT_EQ(rows[3].recording_id, "fault_TIE_1"); +} + +TYPED_TEST(RosbagRetentionParityTest, ACapTieIsBrokenByInsertionOrderNotArbitrarily) { + // The same tie, now deciding which bag gets deleted. Whichever row wins, both + // backends must agree, and the survivor's bag must be the one still on disk. + this->storage_->set_max_rosbags_per_fault(1); + const auto first = this->row("TIECAP", "fault_TIECAP_1", 7000); + this->storage_->store_rosbag_file(first); + const auto second = this->row("TIECAP", "fault_TIECAP_2", 7000); + this->storage_->store_rosbag_file(second); + + const auto rows = this->storage_->get_rosbag_files("TIECAP"); + ASSERT_EQ(rows.size(), 1u); + EXPECT_EQ(rows[0].recording_id, "fault_TIECAP_2"); + EXPECT_TRUE(std::filesystem::exists(second.file_path)); + EXPECT_FALSE(std::filesystem::exists(first.file_path)); +} + +TYPED_TEST(RosbagRetentionParityTest, ABurstRecordingSurvivesWhileASiblingRowReferencesIt) { + // One bag, several faults. The cap evicts A's link but B still holds the bytes. + this->storage_->set_max_rosbags_per_fault(1); + auto shared = this->row("BURST_A", "fault_BURST_1", 1000); + this->storage_->store_rosbag_file(shared); + shared.fault_code = "BURST_B"; + this->storage_->store_rosbag_file(shared); + + const auto shared_path = shared.file_path; + this->storage_->store_rosbag_file(this->row("BURST_A", "fault_BURST_2", 2000)); + + EXPECT_TRUE(std::filesystem::exists(shared_path)) << "the sibling fault still owns the shared bag"; + const auto sibling = this->storage_->get_rosbag_files("BURST_B"); + ASSERT_EQ(sibling.size(), 1u); + EXPECT_EQ(sibling[0].file_path, shared_path); +} + +TYPED_TEST(RosbagRetentionParityTest, RecordingLookupReturnsEveryFaultOfTheBurst) { + // This is what the gateway authorizes against: the union of faults attached to a + // recording. Missing one would 404 a download the entity is entitled to. + this->storage_->set_max_rosbags_per_fault(0); + auto shared = this->row("LOOKUP_A", "fault_LOOKUP_1", 1000); + this->storage_->store_rosbag_file(shared); + shared.fault_code = "LOOKUP_B"; + this->storage_->store_rosbag_file(shared); + shared.fault_code = "LOOKUP_C"; + this->storage_->store_rosbag_file(shared); + + const auto rows = this->storage_->get_rosbag_files_by_recording("fault_LOOKUP_1"); + ASSERT_EQ(rows.size(), 3u); + std::vector codes; + for (const auto & r : rows) { + codes.push_back(r.fault_code); + EXPECT_EQ(r.file_path, shared.file_path); + } + std::sort(codes.begin(), codes.end()); + EXPECT_EQ(codes, (std::vector{"LOOKUP_A", "LOOKUP_B", "LOOKUP_C"})); +} + +TYPED_TEST(RosbagRetentionParityTest, AnUnknownRecordingLooksUpEmptyRatherThanThrowing) { + // The gateway's compatibility path depends on this: empty means "not a recording, + // try it as a fault code", so it must be a normal answer. + EXPECT_TRUE(this->storage_->get_rosbag_files_by_recording("fault_NOPE_1").empty()); + EXPECT_TRUE(this->storage_->get_rosbag_files("NOPE").empty()); + EXPECT_FALSE(this->storage_->get_rosbag_file("NOPE").has_value()); +} + +TYPED_TEST(RosbagRetentionParityTest, DroppingOneLinkLeavesTheFaultsOtherRecordings) { + this->storage_->set_max_rosbags_per_fault(0); + const auto doomed = this->row("DROP", "fault_DROP_1", 1000); + this->storage_->store_rosbag_file(doomed); + this->storage_->store_rosbag_file(this->row("DROP", "fault_DROP_2", 2000)); + + EXPECT_TRUE(this->storage_->drop_rosbag_link("DROP", "fault_DROP_1")); + const auto rows = this->storage_->get_rosbag_files("DROP"); + ASSERT_EQ(rows.size(), 1u); + EXPECT_EQ(rows[0].recording_id, "fault_DROP_2"); + EXPECT_FALSE(std::filesystem::exists(doomed.file_path)); + + EXPECT_FALSE(this->storage_->drop_rosbag_link("DROP", "fault_DROP_1")) << "dropping twice is not an error"; +} + +TYPED_TEST(RosbagRetentionParityTest, DroppingALinkKeepsABagASiblingStillReferences) { + this->storage_->set_max_rosbags_per_fault(0); + auto shared = this->row("DROPSH_A", "fault_DROPSH_1", 1000); + this->storage_->store_rosbag_file(shared); + shared.fault_code = "DROPSH_B"; + this->storage_->store_rosbag_file(shared); + + EXPECT_TRUE(this->storage_->drop_rosbag_link("DROPSH_A", "fault_DROPSH_1")); + EXPECT_TRUE(this->storage_->get_rosbag_files("DROPSH_A").empty()); + EXPECT_EQ(this->storage_->get_rosbag_files("DROPSH_B").size(), 1u); + EXPECT_TRUE(std::filesystem::exists(shared.file_path)); +} + +TYPED_TEST(RosbagRetentionParityTest, DeletingARecordingRemovesEveryLinkAndTheBag) { + // What the quota sweep calls. Deleting by fault code here would wipe the whole + // history of every fault the bag touched. + this->storage_->set_max_rosbags_per_fault(0); + auto shared = this->row("DELREC_A", "fault_DELREC_1", 1000); + this->storage_->store_rosbag_file(shared); + shared.fault_code = "DELREC_B"; + this->storage_->store_rosbag_file(shared); + const auto kept = this->row("DELREC_A", "fault_DELREC_2", 2000); + this->storage_->store_rosbag_file(kept); + + EXPECT_EQ(this->storage_->delete_rosbag_recording("fault_DELREC_1"), 2u); + EXPECT_FALSE(std::filesystem::exists(shared.file_path)); + EXPECT_TRUE(this->storage_->get_rosbag_files("DELREC_B").empty()); + + const auto survivors = this->storage_->get_rosbag_files("DELREC_A"); + ASSERT_EQ(survivors.size(), 1u) << "the other fault's other recording is untouched"; + EXPECT_EQ(survivors[0].recording_id, "fault_DELREC_2"); + EXPECT_TRUE(std::filesystem::exists(kept.file_path)); + + EXPECT_EQ(this->storage_->delete_rosbag_recording("fault_DELREC_1"), 0u); +} + +TYPED_TEST(RosbagRetentionParityTest, DeletingAFaultDropsAllItsRecordings) { + this->storage_->set_max_rosbags_per_fault(0); + const auto a = this->row("DELALL", "fault_DELALL_1", 1000); + const auto b = this->row("DELALL", "fault_DELALL_2", 2000); + this->storage_->store_rosbag_file(a); + this->storage_->store_rosbag_file(b); + + EXPECT_TRUE(this->storage_->delete_rosbag_file("DELALL")); + EXPECT_TRUE(this->storage_->get_rosbag_files("DELALL").empty()); + EXPECT_FALSE(std::filesystem::exists(a.file_path)); + EXPECT_FALSE(std::filesystem::exists(b.file_path)); +} + +TYPED_TEST(RosbagRetentionParityTest, ASharedBagCountsOnceTowardsStorageAcrossSeveralRecordings) { + // The quota sums bytes per path, not per row. N recordings per fault multiplies the + // rows, so a per-row sum would over-report and evict healthy bags under no pressure. + this->storage_->set_max_rosbags_per_fault(0); + auto shared = this->row("QUOTA_A", "fault_QUOTA_1", 1000); + this->storage_->store_rosbag_file(shared); + shared.fault_code = "QUOTA_B"; + this->storage_->store_rosbag_file(shared); + shared.fault_code = "QUOTA_C"; + this->storage_->store_rosbag_file(shared); + this->storage_->store_rosbag_file(this->row("QUOTA_A", "fault_QUOTA_2", 2000)); + + EXPECT_EQ(this->storage_->get_total_rosbag_storage_bytes(), 2048u) << "two distinct bags of 1024, counted once each"; +} + +TYPED_TEST(RosbagRetentionParityTest, GetAllRosbagFilesListsEveryRecordingOldestFirst) { + // The quota sweep walks this list and evicts from the front, so the extra rows this + // change introduces have to appear here in a defined order. + this->storage_->set_max_rosbags_per_fault(0); + this->storage_->store_rosbag_file(this->row("ALL_A", "fault_ALL_1", 3000)); + this->storage_->store_rosbag_file(this->row("ALL_A", "fault_ALL_2", 1000)); + this->storage_->store_rosbag_file(this->row("ALL_B", "fault_ALL_3", 2000)); + + const auto rows = this->storage_->get_all_rosbag_files(); + ASSERT_EQ(rows.size(), 3u); + EXPECT_EQ(rows[0].recording_id, "fault_ALL_2"); + EXPECT_EQ(rows[1].recording_id, "fault_ALL_3"); + EXPECT_EQ(rows[2].recording_id, "fault_ALL_1"); +} + +} // namespace 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 b1ec22a9f..09b6dca7f 100644 --- a/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp +++ b/src/ros2_medkit_fault_manager/test/test_sqlite_storage.cpp @@ -30,6 +30,7 @@ #include "ros2_medkit_msgs/srv/report_fault.hpp" using ros2_medkit_fault_manager::DebounceConfig; +using ros2_medkit_fault_manager::RosbagFileInfo; using ros2_medkit_fault_manager::SqliteFaultStorage; using ros2_medkit_msgs::msg::Fault; using ros2_medkit_msgs::srv::ReportFault; @@ -913,6 +914,209 @@ TEST_F(SqliteFaultStorageTest, ConfirmedAtColumnMigratedIntoOldDatabase) { EXPECT_EQ(read_confirmed_at(temp_db_path_, "NEW"), t.nanoseconds()); } +// --- #620: many recordings per fault ----------------------------------------- + +namespace { + +/// Read PRAGMA index_list and report whether any index came from a table/column +/// UNIQUE constraint (origin 'u'), which is the thing ALTER TABLE cannot drop. +bool has_unique_constraint_index(const std::filesystem::path & db_path, const std::string & table) { + sqlite3 * raw = nullptr; + EXPECT_EQ(sqlite3_open(db_path.string().c_str(), &raw), SQLITE_OK); + sqlite3_stmt * stmt = nullptr; + const std::string sql = "PRAGMA index_list(" + table + ")"; + EXPECT_EQ(sqlite3_prepare_v2(raw, sql.c_str(), -1, &stmt, nullptr), SQLITE_OK); + bool found = false; + while (sqlite3_step(stmt) == SQLITE_ROW) { + const auto * origin = reinterpret_cast(sqlite3_column_text(stmt, 3)); + if (origin != nullptr && std::string(origin) == "u") { + found = true; + } + } + sqlite3_finalize(stmt); + sqlite3_close(raw); + return found; +} + +RosbagFileInfo make_rosbag(const std::string & code, const std::string & path, int64_t created_ns, + size_t bytes = 1024) { + RosbagFileInfo info; + info.fault_code = code; + info.file_path = path; + info.format = "mcap"; + info.duration_sec = 6.0; + info.size_bytes = bytes; + info.created_at_ns = created_ns; + return info; // recording_id deliberately left empty - the backend must fill it +} + +} // namespace + +TEST_F(SqliteFaultStorageTest, LegacyUniqueConstraintIsRebuiltAwayAndRecordingIdBackfilled) { + // A database from before #620: fault_code carries a column-level UNIQUE and there + // is no recording_id column at all. Opening it must rebuild the table, keep every + // row, and derive recording_id from each path's basename. + storage_.reset(); + std::filesystem::remove(temp_db_path_); + { + sqlite3 * raw = nullptr; + ASSERT_EQ(sqlite3_open(temp_db_path_.string().c_str(), &raw), SQLITE_OK); + ASSERT_EQ(sqlite3_exec(raw, + "CREATE TABLE rosbag_files (" + " id INTEGER PRIMARY KEY AUTOINCREMENT," + " fault_code TEXT NOT NULL UNIQUE," + " file_path TEXT NOT NULL," + " format TEXT NOT NULL," + " duration_sec REAL NOT NULL," + " size_bytes INTEGER NOT NULL," + " created_at_ns INTEGER NOT NULL);" + "INSERT INTO rosbag_files (fault_code, file_path, format, duration_sec, size_bytes, " + "created_at_ns) VALUES ('A', '/bags/fault_A_100', 'mcap', 6.0, 10, 100);" + "INSERT INTO rosbag_files (fault_code, file_path, format, duration_sec, size_bytes, " + "created_at_ns) VALUES ('B', '/bags/fault_B_200', 'sqlite3', 6.0, 20, 200);", + nullptr, nullptr, nullptr), + SQLITE_OK); + sqlite3_close(raw); + } + ASSERT_TRUE(has_unique_constraint_index(temp_db_path_, "rosbag_files")) << "fixture must start constrained"; + + storage_ = std::make_unique(temp_db_path_.string()); + + EXPECT_FALSE(has_unique_constraint_index(temp_db_path_, "rosbag_files")) + << "the column-level UNIQUE must be gone, replaced by a named index"; + + const auto all = storage_->get_all_rosbag_files(); + ASSERT_EQ(all.size(), 2u) << "no row may be lost in the rebuild"; + // Oldest first, so the original insertion order survived the copy. + EXPECT_EQ(all[0].fault_code, "A"); + EXPECT_EQ(all[1].fault_code, "B"); + EXPECT_EQ(all[0].recording_id, "fault_A_100") << "backfilled from the path basename"; + EXPECT_EQ(all[1].recording_id, "fault_B_200"); +} + +TEST_F(SqliteFaultStorageTest, ReopeningAMigratedDatabaseIsANoOp) { + // The migration runs on every open, so it has to be idempotent - a second and + // third open must not rebuild again, lose rows or re-backfill. + storage_->store_rosbag_file(make_rosbag("A", "/bags/fault_A_100", 100)); + const auto before = storage_->get_all_rosbag_files(); + + for (int i = 0; i < 2; ++i) { + storage_.reset(); + storage_ = std::make_unique(temp_db_path_.string()); + const auto after = storage_->get_all_rosbag_files(); + ASSERT_EQ(after.size(), before.size()) << "reopen #" << i + 1 << " changed the row count"; + EXPECT_EQ(after[0].recording_id, before[0].recording_id); + EXPECT_FALSE(has_unique_constraint_index(temp_db_path_, "rosbag_files")); + } +} + +TEST_F(SqliteFaultStorageTest, OneFaultKeepsSeveralRecordingsWhenTheCapAllows) { + storage_->set_max_rosbags_per_fault(3); + storage_->store_rosbag_file(make_rosbag("FLAP", "/bags/fault_FLAP_100", 100)); + storage_->store_rosbag_file(make_rosbag("FLAP", "/bags/fault_FLAP_200", 200)); + + const auto rows = storage_->get_rosbag_files("FLAP"); + ASSERT_EQ(rows.size(), 2u) << "the second recording must not replace the first"; + EXPECT_EQ(rows[0].recording_id, "fault_FLAP_200") << "newest first"; + EXPECT_EQ(rows[1].recording_id, "fault_FLAP_100"); + + // get_rosbag_file is "the newest", deterministically. + const auto newest = storage_->get_rosbag_file("FLAP"); + ASSERT_TRUE(newest.has_value()); + EXPECT_EQ(newest->recording_id, "fault_FLAP_200"); +} + +TEST_F(SqliteFaultStorageTest, CapKeepsTheNewestAndUnlinksTheEvictedBag) { + const auto bags = temp_db_path_.parent_path() / (temp_db_path_.stem().string() + "_bags"); + const auto dir_a = bags / "fault_CAP_100"; + const auto dir_b = bags / "fault_CAP_200"; + std::filesystem::create_directories(dir_a); + std::filesystem::create_directories(dir_b); + + storage_->set_max_rosbags_per_fault(1); + storage_->store_rosbag_file(make_rosbag("CAP", dir_a.string(), 100)); + storage_->store_rosbag_file(make_rosbag("CAP", dir_b.string(), 200)); + + const auto rows = storage_->get_rosbag_files("CAP"); + ASSERT_EQ(rows.size(), 1u) << "cap 1 is the historical behaviour: one recording per fault"; + EXPECT_EQ(rows[0].file_path, dir_b.string()); + EXPECT_FALSE(std::filesystem::exists(dir_a)) << "the evicted bag must be unlinked"; + EXPECT_TRUE(std::filesystem::exists(dir_b)); +} + +TEST_F(SqliteFaultStorageTest, EvictingOneFaultsLinkKeepsABagASiblingStillReferences) { + // A burst shares one recording. Evicting it for one fault must not take the bytes + // the other fault still points at. + const auto bags = temp_db_path_.parent_path() / (temp_db_path_.stem().string() + "_bags"); + const auto shared = bags / "fault_SHARED_100"; + const auto later = bags / "fault_A_200"; + std::filesystem::create_directories(shared); + std::filesystem::create_directories(later); + + storage_->set_max_rosbags_per_fault(1); + storage_->store_rosbag_files({make_rosbag("A", shared.string(), 100), make_rosbag("B", shared.string(), 100)}); + // A re-confirms with its own new bag, so A's link to the shared one is evicted. + storage_->store_rosbag_file(make_rosbag("A", later.string(), 200)); + + EXPECT_TRUE(std::filesystem::exists(shared)) << "B still references it"; + const auto b_rows = storage_->get_rosbag_files("B"); + ASSERT_EQ(b_rows.size(), 1u); + EXPECT_EQ(b_rows[0].file_path, shared.string()); +} + +TEST_F(SqliteFaultStorageTest, RecordingLookupReturnsEveryFaultOfTheBurst) { + storage_->store_rosbag_files({make_rosbag("A", "/bags/fault_A_100", 100), make_rosbag("B", "/bags/fault_A_100", 100), + make_rosbag("C", "/bags/fault_A_100", 100)}); + + const auto rows = storage_->get_rosbag_files_by_recording("fault_A_100"); + ASSERT_EQ(rows.size(), 3u) << "the download authorizes against this set"; + EXPECT_EQ(rows[0].fault_code, "A"); + EXPECT_EQ(rows[2].fault_code, "C"); + EXPECT_TRUE(storage_->get_rosbag_files_by_recording("fault_NOPE_1").empty()); +} + +TEST_F(SqliteFaultStorageTest, DeleteRecordingRemovesEveryLinkAndTheBag) { + const auto bags = temp_db_path_.parent_path() / (temp_db_path_.stem().string() + "_bags"); + const auto dir = bags / "fault_A_100"; + std::filesystem::create_directories(dir); + storage_->store_rosbag_files({make_rosbag("A", dir.string(), 100), make_rosbag("B", dir.string(), 100)}); + + EXPECT_EQ(storage_->delete_rosbag_recording("fault_A_100"), 2u); + EXPECT_TRUE(storage_->get_rosbag_files("A").empty()); + EXPECT_TRUE(storage_->get_rosbag_files("B").empty()); + EXPECT_FALSE(std::filesystem::exists(dir)); +} + +TEST_F(SqliteFaultStorageTest, DropLinkLeavesTheOtherFaultsRecordingsAlone) { + storage_->set_max_rosbags_per_fault(0); // unlimited, so nothing is evicted behind our backs + storage_->store_rosbag_file(make_rosbag("A", "/bags/fault_A_100", 100)); + storage_->store_rosbag_file(make_rosbag("A", "/bags/fault_A_200", 200)); + + EXPECT_TRUE(storage_->drop_rosbag_link("A", "fault_A_100")); + const auto rows = storage_->get_rosbag_files("A"); + ASSERT_EQ(rows.size(), 1u) << "only the named link goes"; + EXPECT_EQ(rows[0].recording_id, "fault_A_200"); + EXPECT_FALSE(storage_->drop_rosbag_link("A", "fault_A_100")) << "already gone"; +} + +TEST_F(SqliteFaultStorageTest, DeletingAFaultDropsAllItsRecordings) { + storage_->set_max_rosbags_per_fault(0); + storage_->store_rosbag_file(make_rosbag("A", "/bags/fault_A_100", 100)); + storage_->store_rosbag_file(make_rosbag("A", "/bags/fault_A_200", 200)); + + EXPECT_TRUE(storage_->delete_rosbag_file("A")); + EXPECT_TRUE(storage_->get_rosbag_files("A").empty()) << "auto_cleanup drops the fault's whole history"; +} + +TEST_F(SqliteFaultStorageTest, SharedRecordingStillCountsOnceTowardsStorageWithSeveralRecordings) { + storage_->set_max_rosbags_per_fault(0); + storage_->store_rosbag_files( + {make_rosbag("A", "/bags/fault_A_100", 100, 4096), make_rosbag("B", "/bags/fault_A_100", 100, 4096)}); + storage_->store_rosbag_file(make_rosbag("A", "/bags/fault_A_200", 200, 1024)); + + EXPECT_EQ(storage_->get_total_rosbag_storage_bytes(), 4096u + 1024u) << "bytes belong to file_path, not to the row"; +} + // Snapshot storage tests // @verifies REQ_INTEROP_088 TEST_F(SqliteFaultStorageTest, StoreAndRetrieveSnapshot) { @@ -1052,6 +1256,31 @@ TEST_F(SqliteFaultStorageTest, ClearFaultDeletesAssociatedSnapshots) { // Freeze-frame storage tests // @verifies REQ_INTEROP_088 +TEST_F(SqliteFaultStorageTest, ClearFaultKeepsSnapshotsWhenEvidenceIsRetained) { + using ros2_medkit_fault_manager::SnapshotData; + + rclcpp::Clock clock; + storage_->report_fault_event("KEEP", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "keep", "/n", + clock.now(), default_config()); + storage_->set_retain_snapshots_on_clear(true); + + SnapshotData row; + row.fault_code = "KEEP"; + row.topic = "/t"; + row.message_type = "std_msgs/msg/Float64"; + row.data = R"({"data": 1.0})"; + row.captured_at_ns = 1000; + row.capture_id = 1; + storage_->store_snapshots({row}); + + ASSERT_TRUE(storage_->clear_fault("KEEP")); + + // Recordings survive an acknowledgement once a history is configured, so the + // readings captured beside them have to as well - otherwise the fault is left + // holding bags whose values are gone, which is worse than losing both. + EXPECT_EQ(storage_->get_snapshots("KEEP").size(), 1u); +} + TEST_F(SqliteFaultStorageTest, StoreAndRetrieveFreezeFrame) { using ros2_medkit_fault_manager::FreezeFrameData; rclcpp::Clock clock; @@ -1451,41 +1680,82 @@ TEST_F(SqliteFaultStorageTest, BulkDeleteRemovesTheBurstAndUnlinksTheBagOnce) { // Snapshot limit tests (issue #308) // ============================================================================= -TEST_F(SqliteFaultStorageTest, SnapshotLimitRejectsNewWhenFull) { +TEST_F(SqliteFaultStorageTest, SnapshotCapKeepsTheNewestCaptureWholeAndDropsTheOldest) { using ros2_medkit_fault_manager::SnapshotData; rclcpp::Clock clock; storage_->report_fault_event("MOTOR_OVERHEAT", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "Motor overheated", "/motor_node", clock.now(), default_config()); - // Set limit to 2 snapshots per fault - storage_->set_max_snapshots_per_fault(2); + // Two topics per capture, room for two captures. + storage_->set_max_snapshots_per_fault(4); + + const auto capture = [](int64_t id, int64_t at) { + std::vector rows; + for (const char * topic : {"/motor/temp", "/motor/rpm"}) { + SnapshotData row; + row.fault_code = "MOTOR_OVERHEAT"; + row.topic = topic; + row.message_type = "std_msgs/msg/Float64"; + row.data = R"({"data": 1.0})"; + row.captured_at_ns = at; + row.capture_id = id; + rows.push_back(row); + } + return rows; + }; + + storage_->store_snapshots(capture(1, 1000)); + storage_->store_snapshots(capture(2, 2000)); + storage_->store_snapshots(capture(3, 3000)); - SnapshotData snap1; - snap1.fault_code = "MOTOR_OVERHEAT"; - snap1.topic = "/motor/temp"; - snap1.message_type = "std_msgs/msg/Float64"; - snap1.data = R"({"data": 85.0})"; - snap1.captured_at_ns = 1000; + auto snapshots = storage_->get_snapshots("MOTOR_OVERHEAT"); - SnapshotData snap2 = snap1; - snap2.data = R"({"data": 90.0})"; - snap2.captured_at_ns = 2000; + // The third capture is stored WHOLE and the first goes whole. The old rule + // counted rows and rejected the new one once full, so capture 3 landed with one + // topic present and the other silently missing - a freeze frame with a hole in + // it that reads exactly like "that topic was not publishing". + ASSERT_EQ(snapshots.size(), 4u); + std::set captures; + for (const auto & s : snapshots) { + captures.insert(s.capture_id); + } + EXPECT_EQ(captures, (std::set{2, 3})); + for (int64_t id : {2, 3}) { + EXPECT_EQ(std::count_if(snapshots.begin(), snapshots.end(), + [id](const SnapshotData & s) { + return s.capture_id == id; + }), + 2) + << "capture " << id << " was stored in part"; + } +} - SnapshotData snap3 = snap1; - snap3.data = R"({"data": 95.0})"; - snap3.captured_at_ns = 3000; +TEST_F(SqliteFaultStorageTest, ACaptureLargerThanTheCapIsKeptWholeRatherThanTorn) { + using ros2_medkit_fault_manager::SnapshotData; - storage_->store_snapshot(snap1); - storage_->store_snapshot(snap2); - storage_->store_snapshot(snap3); // Should be rejected + rclcpp::Clock clock; + storage_->report_fault_event("WIDE", ReportFault::Request::EVENT_FAILED, Fault::SEVERITY_ERROR, "many topics", "/n", + clock.now(), default_config()); + storage_->set_max_snapshots_per_fault(2); - auto snapshots = storage_->get_snapshots("MOTOR_OVERHEAT"); - ASSERT_EQ(snapshots.size(), 2u) << "Third snapshot should be rejected (limit=2)"; + std::vector rows; + for (int i = 0; i < 4; ++i) { + SnapshotData row; + row.fault_code = "WIDE"; + row.topic = "/t" + std::to_string(i); + row.message_type = "std_msgs/msg/Float64"; + row.data = "{}"; + row.captured_at_ns = 1000; + row.capture_id = 7; + rows.push_back(row); + } + storage_->store_snapshots(rows); - // Earliest snapshots kept (reject-new strategy), returned newest-first - EXPECT_EQ(snapshots[0].captured_at_ns, 2000); - EXPECT_EQ(snapshots[1].captured_at_ns, 1000); + // The cap is smaller than this fault's topic count. Trimming to it would mean + // storing the reading with holes, which is the failure being fixed; the capture + // stays whole and the operator can see the cap is too small. + EXPECT_EQ(storage_->get_snapshots("WIDE").size(), 4u); } TEST_F(SqliteFaultStorageTest, SnapshotLimitZeroMeansUnlimited) { diff --git a/src/ros2_medkit_gateway/CHANGELOG.rst b/src/ros2_medkit_gateway/CHANGELOG.rst index ec7423f89..de82f8d0f 100644 --- a/src/ros2_medkit_gateway/CHANGELOG.rst +++ b/src/ros2_medkit_gateway/CHANGELOG.rst @@ -4,6 +4,7 @@ Changelog for package ros2_medkit_gateway Forthcoming ----------- +* Rosbag bulk-data is addressed by recording id instead of fault code, so a fault holding several recordings can expose each one. ``GET /{entity}/bulk-data/rosbags`` now emits one descriptor per recording rather than one per fault - a burst that shares a bag used to appear as several entries each reporting the full bag size - and the covered faults move into ``x-medkit.fault_codes`` (was the scalar ``x-medkit.fault_code``). Old URLs keep working: an id that is not a recording is resolved as a fault code and serves that fault's newest recording, which is what it returned before. Authorization is unchanged in effect - a download is allowed when any fault the recording covers is in the entity's source scope, which is exactly the set that could reach it previously (`#620 `_) * Manual asset inventory: a manifest ``assets:`` list and a new ``discovery.inventory.csv_path`` parameter declare assets that no protocol layer can describe (or fully describe). Both paths recognize the canonical names ``id, manufacturer, model, serial, hardware_rev, firmware, endpoint, role, area`` plus the shared aliases (``serial_number``, ``hardware_revision`` / ``hw_rev``, ``firmware_version`` / ``fw``) and keep any other column / key as an extra; RFC-4180-style quoting is honored. Each asset becomes a Component with ``source = "inventory"`` and a structured asset identity carrying per-field provenance, appended to the base manifest on every load / reload and merged into the tree by id alongside protocol-discovered structure; ``area`` places the asset under an Area, without it the asset appears only in the flat component list. CSV rows never fail the load: rows without an ``id`` are skipped with a warning, for duplicate ids the first row wins, a row whose id is already a manifest component keeps the manifest definition (the row's identity is folded in as gap-fill), and an unknown ``area`` is dropped with a warning. The CSV is size-capped at 1 MiB before being read; a missing file is skipped with a warning (mirrors ``fragments_dir``), while an unreadable or malformed one fails the load / reload. Requires a manifest-backed discovery mode (``manifest_only`` / ``hybrid`` with ``discovery.manifest_path`` set); empty = disabled (default) (`#490 `_) 0.6.0 (2026-06-22) diff --git a/src/ros2_medkit_gateway/README.md b/src/ros2_medkit_gateway/README.md index e5be493a9..e4c02a75c 100644 --- a/src/ros2_medkit_gateway/README.md +++ b/src/ros2_medkit_gateway/README.md @@ -817,7 +817,7 @@ Real-time fault event stream using Server-Sent Events (SSE). Clients receive ins - **Automatic reconnection**: Supports `Last-Event-ID` header for seamless reconnection - **Keepalive**: Sends `:keepalive` comment every 30 seconds to prevent timeouts - **Event buffer**: Buffers up to 100 recent events for reconnecting clients. Under overflow the buffer evicts entries every live client has already received, then `fault_updated` entries superseded by a newer event for the same fault code - a lagging client still converges on the current state of every fault and loses no status transition. Anything beyond that (a superseded transition, or an event with no newer sibling) is genuinely lost for lagging clients: those losses are counted and logged as drops, and an affected client should refetch `GET /api/v1/faults` to resynchronize -- **Entity context (SOVD payload extension)**: When the gateway can resolve the fault's first reporting source back to an entity, the payload carries an `x-medkit` object with `entity_type` and `entity_id` fields so consumers can hit `/{entity_type}/{entity_id}/bulk-data/rosbags/{fault_code}` directly without enumerating entities +- **Entity context (SOVD payload extension)**: When the gateway can resolve the fault's first reporting source back to an entity, the payload carries an `x-medkit` object with `entity_type` and `entity_id` fields so consumers can hit `/{entity_type}/{entity_id}/bulk-data/rosbags/{fault_code}` directly without enumerating entities. That address serves the fault's newest recording; to reach an older one, list `/bulk-data/rosbags` and use the descriptor `id` (the recording id) - **Correlation payload**: when a root-cause event auto-clears correlated symptom faults, the payload carries their codes in `auto_cleared_codes` (omitted when empty); those symptoms get no event of their own **Event Types:** diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp index ef4266420..5c00cd0d7 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/handlers/bulkdata_handlers.hpp @@ -15,6 +15,7 @@ #pragma once #include +#include #include #include @@ -130,19 +131,63 @@ std::vector compute_bulkdata_source_filters(const ThreadSafeEntityC const EntityInfo & entity); /** - * @brief Shared-recording identifier for a rosbag descriptor. + * @brief Identity of a rosbag recording, derived from its path. * - * Faults confirmed in one burst share a single recording, and each fault gets - * its own descriptor with the full bag size. The bag directory basename (e.g. - * ``fault_MOTOR_OVERHEAT_1738662600000``) identifies the recording, so clients - * can group descriptors that serve the same bytes. Empty when the path is - * empty or has no usable basename. + * The bag directory basename (e.g. ``fault_MOTOR_OVERHEAT_1738662600000``) is + * the recording's public name: it addresses the bag under + * ``/bulk-data/rosbags/{id}`` and groups the link rows that serve the same + * bytes. The fault manager stores the same value; this derivation is the + * fallback for a peer or a replay that predates the stored field. Empty when + * the path is empty or has no usable basename. * * @param file_path Bag path as stored by the fault manager (directory) * @return Basename of the bag directory, or empty string */ std::string rosbag_recording_id(const std::string & file_path); +/** + * @brief Fault codes a rosbag download is authorized against. + * + * A recording is shared by every fault of a burst, so ownership is the union + * over those faults rather than a single code: the entity that owns any one of + * them may download the bag. That grants nothing new - before recordings had + * their own identity, each of those faults already addressed its own copy of + * the same bytes - it only renames the door. + * + * When the wire carries no ``fault_codes`` the response came from a peer that + * predates the field, where the addressed id *was* the fault code; authorizing + * against the requested id then reproduces the previous check exactly. + * + * @param rosbag_data Rosbag response from the fault manager + * @param requested_id The ``{file_id}`` path segment the client asked for + * @return Non-empty list of fault codes to test against the entity's scope + */ +std::vector rosbag_attached_fault_codes(const nlohmann::json & rosbag_data, + const std::string & requested_id); + +/** + * @brief Fold rosbag link rows into one descriptor per recording. + * + * The fault manager returns one row per ``(fault, recording)`` link, so a burst + * of correlated faults arrives as several rows naming one bag, and one fault + * can name several bags. Emitting one descriptor per row would repeat an id and + * report the full bag size once per attached fault, which reads as several bags + * worth of storage. Rows are therefore grouped by recording, the attached codes + * collected into ``x-medkit.fault_codes`` (sorted, so the output is stable), and + * the recording dated by the earliest fault of its burst. Rows with neither a + * recording id nor a usable path are dropped - nothing could address them. + * + * Order follows first appearance, which is the order the fault manager listed + * the rows in. + * + * @param rows Rosbag rows as returned by the fault manager + * @param faults_by_code Faults keyed by code, for timestamp enrichment + * @return One descriptor per distinct recording + */ +std::vector +fold_rosbag_rows_into_descriptors(const std::vector & rows, + const std::unordered_map & faults_by_code); + } // namespace detail } // namespace handlers diff --git a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp index f36f20a13..7cbabcf60 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp @@ -112,6 +112,106 @@ std::string rosbag_recording_id(const std::string & file_path) { return p.filename().string(); } +std::vector rosbag_attached_fault_codes(const nlohmann::json & rosbag_data, + const std::string & requested_id) { + if (rosbag_data.contains("fault_codes") && rosbag_data["fault_codes"].is_array()) { + // Element by element rather than get>(): the array is built + // from a ROS string vector today and cannot hold anything else, but this + // function exists to tolerate a peer that predates the field, and a + // whole-array conversion throws on the first non-string it meets. + std::vector codes; + for (const auto & code : rosbag_data["fault_codes"]) { + if (code.is_string()) { + codes.push_back(code.get()); + } + } + if (!codes.empty()) { + return codes; + } + } + // No list on the wire: a peer that predates the field, where the addressed id + // was the fault code itself. Authorizing against it reproduces the old check. + return {requested_id}; +} + +std::vector +fold_rosbag_rows_into_descriptors(const std::vector & rows, + const std::unordered_map & faults_by_code) { + struct RecordingEntry { + std::string recording_id; + std::string format; + uint64_t size_bytes{0}; + double duration_sec{0.0}; + int64_t created_at_ns{0}; + std::vector fault_codes; + }; + std::vector recordings; + std::unordered_map index_by_recording; + + for (const auto & row : rows) { + // The fault manager supplies the recording id; falling back to the path + // basename covers a peer or a replay predating that field. + std::string recording_id = row.value("recording_id", ""); + if (recording_id.empty()) { + recording_id = rosbag_recording_id(row.value("file_path", "")); + } + if (recording_id.empty()) { + continue; // nothing addressable - a row with neither id nor path + } + + const std::string fault_code = row.value("fault_code", ""); + int64_t created_at_ns = 0; + if (auto it = faults_by_code.find(fault_code); it != faults_by_code.end()) { + const double first_occurred = it->second.value("first_occurred", 0.0); + created_at_ns = static_cast(first_occurred * 1'000'000'000); + } + + if (auto found = index_by_recording.find(recording_id); found != index_by_recording.end()) { + auto & entry = recordings[found->second]; + entry.fault_codes.push_back(fault_code); + // Earliest fault of the burst dates the recording. + if (created_at_ns != 0 && (entry.created_at_ns == 0 || created_at_ns < entry.created_at_ns)) { + entry.created_at_ns = created_at_ns; + } + continue; + } + + RecordingEntry entry; + entry.recording_id = recording_id; + // Default to sqlite3 (the historical FaultManager default) when a bag predates + // the persisted format field; the per-bag metadata normally carries the real one. + entry.format = row.value("format", "sqlite3"); + entry.size_bytes = row.value("size_bytes", uint64_t{0}); + entry.duration_sec = row.value("duration_sec", 0.0); + entry.created_at_ns = created_at_ns; + entry.fault_codes.push_back(fault_code); + index_by_recording.emplace(recording_id, recordings.size()); + recordings.push_back(std::move(entry)); + } + + std::vector descriptors; + descriptors.reserve(recordings.size()); + for (auto & entry : recordings) { + std::sort(entry.fault_codes.begin(), entry.fault_codes.end()); + entry.fault_codes.erase(std::unique(entry.fault_codes.begin(), entry.fault_codes.end()), entry.fault_codes.end()); + + dto::BulkDataDescriptor descriptor; + descriptor.id = entry.recording_id; + descriptor.name = entry.recording_id + " recording " + format_timestamp_ns(entry.created_at_ns); + descriptor.mimetype = BulkDataHandlers::get_rosbag_mimetype(entry.format); + descriptor.size = entry.size_bytes; + descriptor.creation_date = format_timestamp_ns(entry.created_at_ns); + descriptor.x_medkit = nlohmann::json{{"fault_codes", entry.fault_codes}, + {"duration_sec", entry.duration_sec}, + {"format", entry.format}, + // Redundant with the descriptor id, kept because + // clients already group on it. + {"recording_id", entry.recording_id}}; + descriptors.push_back(std::move(descriptor)); + } + return descriptors; +} + std::vector compute_bulkdata_source_filters(const ThreadSafeEntityCache & cache, const EntityInfo & entity) { // One rule for every entity type: the same fault-scope resolution that drives @@ -235,44 +335,7 @@ BulkDataHandlers::list_descriptors(const http::TypedRequest & req) { } } - dto::Collection response; - for (const auto & rosbag : all_rosbags) { - std::string fault_code = rosbag.value("fault_code", ""); - // Default to sqlite3 (the historical FaultManager default) when a bag predates - // the persisted format field; the per-bag metadata normally carries the real one. - std::string format = rosbag.value("format", "sqlite3"); - uint64_t size_bytes = rosbag.value("size_bytes", uint64_t{0}); - double duration_sec = rosbag.value("duration_sec", 0.0); - - // Use fault_code as bulk_data_id - std::string bulk_data_id = fault_code; - - // Get timestamp from fault if available - int64_t created_at_ns = 0; - auto it = fault_map.find(fault_code); - if (it != fault_map.end()) { - double first_occurred = it->second.value("first_occurred", 0.0); - created_at_ns = static_cast(first_occurred * 1'000'000'000); - } - - dto::BulkDataDescriptor descriptor; - descriptor.id = bulk_data_id; - descriptor.name = fault_code + " recording " + format_timestamp_ns(created_at_ns); - descriptor.mimetype = get_rosbag_mimetype(format); - descriptor.size = size_bytes; - descriptor.creation_date = format_timestamp_ns(created_at_ns); - json x_medkit{{"fault_code", fault_code}, {"duration_sec", duration_sec}, {"format", format}}; - // Faults of one burst share a recording and each descriptor reports the - // full bag size; recording_id lets clients group the descriptors that - // serve the same bytes. - std::string recording_id = detail::rosbag_recording_id(rosbag.value("file_path", "")); - if (!recording_id.empty()) { - x_medkit["recording_id"] = recording_id; - } - descriptor.x_medkit = std::move(x_medkit); - response.items.push_back(std::move(descriptor)); - } - return response; + return dto::Collection{detail::fold_rosbag_rows_into_descriptors(all_rosbags, fault_map)}; } // === Non-rosbag categories: served via BulkDataStore === @@ -342,24 +405,37 @@ http::Result BulkDataHandlers::download(const http::TypedR if (category == "rosbags") { // === Rosbags: served via FaultManager === + // + // The id is a recording id. A pre-#620 URL carrying a fault code still resolves: + // the fault manager tries the recording first and falls back to "the newest + // recording of this fault", which is what a fault-code URL has always returned. + // Both routes end up holding a recording before anything is authorized, so there + // is exactly one authorization semantic, not two. auto fault_mgr = ctx_.node()->get_fault_manager(); - std::string fault_code = bulk_data_id; - auto rosbag_result = fault_mgr->get_rosbag(fault_code); + auto rosbag_result = fault_mgr->get_rosbag(bulk_data_id); if (!rosbag_result.success || !rosbag_result.data.contains("file_path")) { return tl::unexpected( make_error(404, ERR_RESOURCE_NOT_FOUND, "Bulk-data not found", json{{"bulk_data_id", bulk_data_id}})); } - // Security check: the bag belongs to this entity only when the fault it - // was captured for is within the entity's source scope. Read the fault - // once and test with the shared boundary-aware matcher - the transport's - // get_fault(code, source) check is a raw prefix match, so app id "plc" - // would claim the assets of "plc_line1". + // Security check: the bag belongs to this entity when ANY fault it was captured + // for is within the entity's source scope. Union rather than a single code + // because a burst shares one recording, and each of those faults already had its + // own downloadable copy of it before - so this grants nothing new, it only + // renames the door. Tested with the shared boundary-aware matcher: the + // transport's get_fault(code, source) check is a raw prefix match, so app id + // "plc" would otherwise claim the assets of "plc_line1". auto source_filters = get_source_filters(entity); std::set scope(source_filters.begin(), source_filters.end()); - auto fault_result = fault_mgr->get_fault(fault_code, ""); - if (!fault_result.success || !faults::fault_in_source_scope(fault_result.data, scope)) { + + const auto attached_codes = detail::rosbag_attached_fault_codes(rosbag_result.data, bulk_data_id); + + const bool authorized = std::any_of(attached_codes.begin(), attached_codes.end(), [&](const std::string & code) { + auto fault_result = fault_mgr->get_fault(code, ""); + return fault_result.success && faults::fault_in_source_scope(fault_result.data, scope); + }); + if (!authorized) { return tl::unexpected(make_error(404, ERR_RESOURCE_NOT_FOUND, "Bulk-data not found for this entity", json{{"entity_id", path_info->entity_id}})); } @@ -369,7 +445,9 @@ http::Result BulkDataHandlers::download(const http::TypedR // format field; metadata normally carries the real one persisted at capture time. std::string format = rosbag_result.data.value("format", "sqlite3"); mimetype = get_rosbag_mimetype(format); - filename = fault_code + "." + format; + // Named after the recording that was actually served, which for a compatibility + // URL is not the segment the client sent. + filename = rosbag_result.data.value("recording_id", bulk_data_id) + "." + format; // Rosbag2 emits a directory layout - resolve the inner db3/mcap file. actual_path = resolve_rosbag_file_path(file_path); diff --git a/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp index 9f3e33c78..76cc4cb6d 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp @@ -335,25 +335,33 @@ dto::FaultDetail FaultHandlers::build_sovd_fault_response(const json & fault_jso snap["x-medkit"]["source_timestamp"] = s["source_timestamp"]; } } else if (snapshot_type == "rosbag") { - // Build absolute URI using entity path + fault_code as the bulk-data ID. - // This must match the download handler which looks up rosbags by fault_code, - // and list_descriptors which also uses fault_code as the descriptor ID. - // A malformed rosbag snapshot missing fault_code is a transport-side - // bug: we still fall back to the parent fault's code to preserve a - // usable bulk-data URI, but we log a WARN so operators notice. - std::string snap_fault_code; - if (s.contains("fault_code") && s["fault_code"].is_string()) { - snap_fault_code = s["fault_code"].get(); - } else { + // Build the absolute URI from the RECORDING id the transport supplied. It + // must match list_descriptors, which uses the same value as the descriptor + // id, and the download handler, which resolves it back to a recording. + // + // No fallback to the parent fault code any more. A fault can hold several + // recordings, so substituting its code would advertise one URL for all of + // them and serve whichever the compatibility path happened to pick. + // + // With no addressable id there is no snapshot to report: a rosbag entry + // is a pointer to bytes, and one that points nowhere is not degraded + // information, it is a download button that cannot work. Every rosbag + // snapshot this endpoint has ever emitted carried a bulk_data_uri, and + // consumers dereference it unguarded; dropping the entry keeps that + // invariant rather than inventing a new shape for them to crash on. + // Same rule as the bulk-data listing, which also drops an unaddressable + // row instead of advertising it. + if (!s.contains("bulk_data_id") || !s["bulk_data_id"].is_string() || + s["bulk_data_id"].get().empty()) { RCLCPP_WARN(HandlerContext::logger(), - "Rosbag snapshot missing 'fault_code' field; falling back to parent fault code '%s' for entity " - "'%s'", + "Rosbag snapshot for fault '%s' on entity '%s' carries no 'bulk_data_id'; omitting it, as " + "nothing could address the recording. This is a transport-side bug.", fault_code.c_str(), entity_path.c_str()); - snap_fault_code = fault_code; + continue; } std::string bulk_data_uri = entity_path; bulk_data_uri += "/bulk-data/rosbags/"; - bulk_data_uri += snap_fault_code; + bulk_data_uri += s["bulk_data_id"].get(); snap["bulk_data_uri"] = std::move(bulk_data_uri); if (s.contains("size_bytes")) { snap["size_bytes"] = s["size_bytes"]; @@ -364,6 +372,14 @@ dto::FaultDetail FaultHandlers::build_sovd_fault_response(const json & fault_jso if (s.contains("format")) { snap["format"] = s["format"]; } + // Same x-medkit.captured_at shape the freeze frame emits, because that + // is the field the UIs already render. Zero means the recording predates + // the timestamp being carried, and an epoch date on screen would be a + // worse answer than none, so the key is left out. + const int64_t captured_at_ns = s.value("captured_at_ns", static_cast(0)); + if (captured_at_ns > 0) { + snap["x-medkit"] = {{"captured_at", to_iso8601_ns(captured_at_ns)}}; + } } snapshots.push_back(snap); diff --git a/src/ros2_medkit_gateway/src/http/handlers/sse_fault_handler.cpp b/src/ros2_medkit_gateway/src/http/handlers/sse_fault_handler.cpp index 63ab647f3..1f0d9d537 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/sse_fault_handler.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/sse_fault_handler.cpp @@ -476,7 +476,10 @@ std::string SSEFaultHandler::format_sse_event(const QueuedEvent & queued) { // SOVD payload extension: nest ``entity_type`` / ``entity_id`` under the // ``x-medkit`` response-extension object so global-stream consumers can // hit ``/{entity_type}/{entity_id}/bulk-data/rosbags/{fault_code}`` - // directly instead of HEAD-probing every entity. Flat ``x-medkit-*`` + // directly instead of HEAD-probing every entity. That address is a + // compatibility one since #620 - bags are keyed by recording id now - and + // serves the fault's newest recording, which is what a stream consumer + // reacting to the event it just received wants. Flat ``x-medkit-*`` // names are reserved for endpoint paths (``/x-medkit-graph``) and error // codes, not payload fields. if (queued.entity) { diff --git a/src/ros2_medkit_gateway/src/ros2/conversions/fault_msg_conversions.cpp b/src/ros2_medkit_gateway/src/ros2/conversions/fault_msg_conversions.cpp index a22906ac3..9d6833187 100644 --- a/src/ros2_medkit_gateway/src/ros2/conversions/fault_msg_conversions.cpp +++ b/src/ros2_medkit_gateway/src/ros2/conversions/fault_msg_conversions.cpp @@ -85,10 +85,18 @@ nlohmann::json environment_data_to_json(const ros2_medkit_msgs::msg::Environment snap["captured_at_ns"] = s.captured_at_ns; } else if (s.type == "rosbag") { snap["snapshot_type"] = "rosbag"; - snap["fault_code"] = s.bulk_data_id; + // Carries a RECORDING id now, so it is named after what it is. Calling it + // "fault_code" while it holds something else is how the next reader builds a + // URL out of the wrong value. + snap["bulk_data_id"] = s.bulk_data_id; snap["size_bytes"] = s.size_bytes; snap["duration_sec"] = s.duration_sec; snap["format"] = s.format; + // The fault manager has always set this; only the freeze-frame branch + // forwarded it, so every rosbag reached the UI with no capture time and + // rendered "N/A". With one recording per fault that was cosmetic. With + // several it is the only thing that tells the occurrences apart. + snap["captured_at_ns"] = s.captured_at_ns; // bulk_data_uri intentionally omitted - handler appends per-request URL } diff --git a/src/ros2_medkit_gateway/src/ros2/transports/ros2_fault_service_transport.cpp b/src/ros2_medkit_gateway/src/ros2/transports/ros2_fault_service_transport.cpp index bcaa6b3c4..155a7cbc1 100644 --- a/src/ros2_medkit_gateway/src/ros2/transports/ros2_fault_service_transport.cpp +++ b/src/ros2_medkit_gateway/src/ros2/transports/ros2_fault_service_transport.cpp @@ -406,7 +406,12 @@ FaultResult Ros2FaultServiceTransport::get_snapshots(const std::string & fault_c FaultResult Ros2FaultServiceTransport::get_rosbag(const std::string & fault_code) { FaultResult result; + // The parameter is the bulk-data id, which is a recording id. It is sent in BOTH + // fields: the fault manager prefers recording_id and falls back to fault_code, + // which is what keeps a pre-#620 URL (and every existing .test.py that calls this + // service with a fault code) working unchanged. auto request = std::make_shared(); + request->recording_id = fault_code; request->fault_code = fault_code; auto response = invoke_fault_service( @@ -420,10 +425,9 @@ FaultResult Ros2FaultServiceTransport::get_rosbag(const std::string & fault_code result.success = response->success; if (response->success) { - result.data = {{"file_path", response->file_path}, - {"format", response->format}, - {"duration_sec", response->duration_sec}, - {"size_bytes", response->size_bytes}}; + result.data = {{"file_path", response->file_path}, {"recording_id", response->recording_id}, + {"fault_codes", response->fault_codes}, {"format", response->format}, + {"duration_sec", response->duration_sec}, {"size_bytes", response->size_bytes}}; } else { result.error_message = response->error_message; } @@ -450,15 +454,14 @@ FaultResult Ros2FaultServiceTransport::list_rosbags(const std::string & entity_f if (response->success) { // The response uses parallel arrays. Trust nothing about the remote // service: a server bug or schema drift could ship arrays of different - // lengths, and indexing past end-of-vector is UB. Take the shortest - // length and surface the mismatch rather than silently truncate. + // lengths, and indexing past end-of-vector is UB. Any array that does not + // match fault_codes is reported as a mismatch rather than silently truncated. const size_t n = response->fault_codes.size(); - const size_t shortest = std::min({n, response->file_paths.size(), response->formats.size(), - response->durations_sec.size(), response->sizes_bytes.size()}); - if (shortest != n || response->file_paths.size() != n || response->formats.size() != n || + if (response->recording_ids.size() != n || response->file_paths.size() != n || response->formats.size() != n || response->durations_sec.size() != n || response->sizes_bytes.size() != n) { result.success = false; result.error_message = "ListRosbags response has mismatched array sizes (fault_codes=" + std::to_string(n) + + ", recording_ids=" + std::to_string(response->recording_ids.size()) + ", file_paths=" + std::to_string(response->file_paths.size()) + ", formats=" + std::to_string(response->formats.size()) + ", durations_sec=" + std::to_string(response->durations_sec.size()) + @@ -468,6 +471,7 @@ FaultResult Ros2FaultServiceTransport::list_rosbags(const std::string & entity_f json rosbags = json::array(); for (size_t i = 0; i < n; ++i) { rosbags.push_back({{"fault_code", response->fault_codes[i]}, + {"recording_id", response->recording_ids[i]}, {"file_path", response->file_paths[i]}, {"format", response->formats[i]}, {"duration_sec", response->durations_sec[i]}, diff --git a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp index 21fad217a..3923f0516 100644 --- a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp @@ -14,9 +14,12 @@ #include +#include #include #include #include +#include +#include #include "ros2_medkit_gateway/core/discovery/models/app.hpp" #include "ros2_medkit_gateway/core/discovery/models/area.hpp" @@ -30,6 +33,8 @@ #include "ros2_medkit_gateway/core/models/thread_safe_entity_cache.hpp" using namespace ros2_medkit_gateway; +// No `using json = nlohmann::json` here: the namespace pulled in above already +// declares that alias, and redeclaring it shadows it. using ros2_medkit_gateway::handlers::BulkDataHandlers; class BulkDataHandlersTest : public ::testing::Test { @@ -75,9 +80,9 @@ TEST_F(BulkDataHandlersTest, GetRosbagMimetypeCasesSensitive) { } // === Shared-recording identifier tests === -// Faults of one burst share a recording; each fault's descriptor carries -// x-medkit.recording_id = the bag directory basename, so clients can group -// the descriptors that serve the same bytes. +// The bag directory basename is the recording's public name: it addresses the +// bag under /bulk-data/rosbags/{id} and groups the link rows serving the same +// bytes. TEST_F(BulkDataHandlersTest, RecordingIdIsTheBagDirectoryBasename) { EXPECT_EQ(handlers::detail::rosbag_recording_id("/var/bags/fault_MOTOR_OVERHEAT_1738664999000"), @@ -98,6 +103,114 @@ TEST_F(BulkDataHandlersTest, RecordingIdToleratesTrailingSlashAndEmptyPath) { EXPECT_EQ(handlers::detail::rosbag_recording_id(""), ""); } +// === Descriptor folding tests === +// The fault manager returns one row per (fault, recording) link. A burst of +// correlated faults is several rows naming one bag, and one fault holding a +// history is several rows with distinct bags. Both shapes have to come out as +// one descriptor per recording. + +namespace { + +json rosbag_row(const std::string & fault_code, const std::string & recording_id, uint64_t size_bytes = 1024) { + return json{{"fault_code", fault_code}, {"recording_id", recording_id}, {"file_path", "/var/bags/" + recording_id}, + {"format", "mcap"}, {"duration_sec", 5.0}, {"size_bytes", size_bytes}}; +} + +json fault_at(double first_occurred) { + return json{{"first_occurred", first_occurred}}; +} + +} // namespace + +TEST_F(BulkDataHandlersTest, OneFaultWithSeveralRecordingsYieldsOneDescriptorEach) { + // The feature: a flapping fault keeps a history, and every recording in it + // has to be separately addressable. + const std::vector rows{rosbag_row("FLAP", "fault_FLAP_3"), rosbag_row("FLAP", "fault_FLAP_2"), + rosbag_row("FLAP", "fault_FLAP_1")}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors(rows, {}); + ASSERT_EQ(descriptors.size(), 3u); + EXPECT_EQ(descriptors[0].id, "fault_FLAP_3") << "order follows the fault manager's listing"; + EXPECT_EQ(descriptors[1].id, "fault_FLAP_2"); + EXPECT_EQ(descriptors[2].id, "fault_FLAP_1"); +} + +TEST_F(BulkDataHandlersTest, ABurstCollapsesToOneDescriptorCarryingEveryFault) { + // Three rows, one bag. Emitting three items would repeat the id and report + // the bag's size three times, which reads as three bags worth of storage. + const std::vector rows{rosbag_row("ROOT_CAUSE", "fault_ROOT_CAUSE_17"), + rosbag_row("DOWNSTREAM_B", "fault_ROOT_CAUSE_17"), + rosbag_row("DOWNSTREAM_A", "fault_ROOT_CAUSE_17")}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors(rows, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].id, "fault_ROOT_CAUSE_17"); + EXPECT_EQ(descriptors[0].size, 1024u) << "the bag is counted once, not once per attached fault"; + + ASSERT_TRUE(descriptors[0].x_medkit.has_value()); + const auto & x = *descriptors[0].x_medkit; + ASSERT_TRUE(x.contains("fault_codes")); + EXPECT_EQ(x["fault_codes"], (json{"DOWNSTREAM_A", "DOWNSTREAM_B", "ROOT_CAUSE"})) << "sorted, so output is stable"; + EXPECT_EQ(x["recording_id"], "fault_ROOT_CAUSE_17"); + EXPECT_EQ(x["format"], "mcap"); + EXPECT_DOUBLE_EQ(x["duration_sec"].get(), 5.0); +} + +TEST_F(BulkDataHandlersTest, TheSameFaultTwiceOnOneRecordingIsNotListedTwice) { + // Two source filters can both resolve to the same app, so the same row can + // arrive twice. A repeated code in fault_codes would be visible in the API. + const std::vector rows{rosbag_row("DUP", "fault_DUP_1"), rosbag_row("DUP", "fault_DUP_1")}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors(rows, {}); + ASSERT_EQ(descriptors.size(), 1u); + ASSERT_TRUE(descriptors[0].x_medkit.has_value()); + EXPECT_EQ((*descriptors[0].x_medkit)["fault_codes"], (json{"DUP"})); +} + +TEST_F(BulkDataHandlersTest, ARecordingIsDatedByTheEarliestFaultOfItsBurst) { + // Downstream faults confirm after the root cause, and the recording covers + // the whole burst, so the earliest is the honest creation date. + const std::vector rows{rosbag_row("DOWNSTREAM", "fault_ROOT_9"), rosbag_row("ROOT", "fault_ROOT_9")}; + const std::unordered_map faults{{"DOWNSTREAM", fault_at(1700000900.0)}, + {"ROOT", fault_at(1700000000.0)}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors(rows, faults); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].creation_date, format_timestamp_ns(int64_t{1700000000} * 1'000'000'000)); +} + +TEST_F(BulkDataHandlersTest, DescriptorIdFallsBackToTheBasenameWhenTheRowHasNoRecordingId) { + // A peer or a replay predating the stored field still has to be addressable. + const std::vector rows{json{{"fault_code", "OLD"}, {"file_path", "/var/bags/fault_OLD_5"}, {"format", "mcap"}}}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors(rows, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].id, "fault_OLD_5"); +} + +TEST_F(BulkDataHandlersTest, ARowWithNeitherIdNorPathIsDroppedRatherThanAdvertised) { + // An empty id would render as /bulk-data/rosbags/ - a 404 the client cannot + // act on. Better absent than advertised and broken. + const std::vector rows{json{{"fault_code", "GHOST"}, {"format", "mcap"}}, rosbag_row("REAL", "fault_REAL_1")}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors(rows, {}); + ASSERT_EQ(descriptors.size(), 1u); + EXPECT_EQ(descriptors[0].id, "fault_REAL_1"); +} + +TEST_F(BulkDataHandlersTest, DistinctRecordingsEachReportTheirOwnSize) { + const std::vector rows{rosbag_row("A", "fault_A_1", 2048), rosbag_row("B", "fault_B_1", 4096)}; + + const auto descriptors = handlers::detail::fold_rosbag_rows_into_descriptors(rows, {}); + ASSERT_EQ(descriptors.size(), 2u); + EXPECT_EQ(descriptors[0].size, 2048u); + EXPECT_EQ(descriptors[1].size, 4096u); +} + +TEST_F(BulkDataHandlersTest, NoRowsYieldsNoDescriptors) { + EXPECT_TRUE(handlers::detail::fold_rosbag_rows_into_descriptors({}, {}).empty()); +} + // === Shared timestamp utility tests === // @verifies REQ_INTEROP_071 @@ -501,6 +614,99 @@ TEST_F(BulkDataSourceFiltersTest, FunctionWithComponentHostResolvesComponentApps // exact match or '/'-boundary prefix only. A raw prefix match (the transport's // get_fault(code, source) semantics) would let app id "plc" claim the bag of // "plc_line1". +// === Download authorization tests === +// A recording is shared by a whole burst, so ownership is the union over its +// attached faults. The scope matcher itself is unchanged and pinned below; what +// is new is which codes get fed to it. + +TEST_F(BulkDataSourceFiltersTest, AttachedFaultCodesComeFromTheRecordingNotTheUrl) { + const nlohmann::json rosbag = {{"file_path", "/var/bags/fault_ROOT_1"}, + {"recording_id", "fault_ROOT_1"}, + {"fault_codes", {"ROOT", "DOWNSTREAM_A", "DOWNSTREAM_B"}}}; + + EXPECT_EQ(handlers::detail::rosbag_attached_fault_codes(rosbag, "fault_ROOT_1"), + (std::vector{"ROOT", "DOWNSTREAM_A", "DOWNSTREAM_B"})); +} + +TEST_F(BulkDataSourceFiltersTest, AttachedFaultCodesFallBackToTheRequestedIdOnAnOlderPeer) { + // The compatibility path: the id addressed was the fault code, and a peer + // that predates the field sends no list. Authorizing against the requested id + // is exactly the check that shipped before. + const nlohmann::json rosbag = {{"file_path", "/var/bags/fault_MOTOR_1"}}; + EXPECT_EQ(handlers::detail::rosbag_attached_fault_codes(rosbag, "MOTOR_OVERHEAT"), + (std::vector{"MOTOR_OVERHEAT"})); +} + +TEST_F(BulkDataSourceFiltersTest, AttachedFaultCodesFallBackOnAnEmptyOrMalformedList) { + // Never return empty: an empty list makes any_of vacuously false, which would + // 404 a download the entity owns. + const nlohmann::json empty_list = {{"fault_codes", nlohmann::json::array()}}; + EXPECT_EQ(handlers::detail::rosbag_attached_fault_codes(empty_list, "X"), (std::vector{"X"})); + + const nlohmann::json not_an_array = {{"fault_codes", "X"}}; + EXPECT_EQ(handlers::detail::rosbag_attached_fault_codes(not_an_array, "X"), (std::vector{"X"})); +} + +TEST_F(BulkDataSourceFiltersTest, ABurstRecordingIsOwnedByAnyEntityOwningOneOfItsFaults) { + // One bag, three faults, two apps. Each app reaches the bag through its own + // fault - which is what it could already do when the bag was addressed by + // fault code. + App plc; + plc.id = "plc"; + plc.external = true; + App vision; + vision.id = "vision"; + vision.external = true; + + ThreadSafeEntityCache cache; + cache.update_all({}, {}, {plc, vision}, {}); + + auto plc_entity = make_entity_info(EntityType::APP, "plc", "", ""); + auto plc_filters = handlers::detail::compute_bulkdata_source_filters(cache, plc_entity); + std::set plc_scope(plc_filters.begin(), plc_filters.end()); + + const std::vector burst_faults = {nlohmann::json{{"reporting_sources", {"vision"}}}, + nlohmann::json{{"reporting_sources", {"plc"}}}}; + + const bool plc_authorized = std::any_of(burst_faults.begin(), burst_faults.end(), [&](const nlohmann::json & f) { + return faults::fault_in_source_scope(f, plc_scope); + }); + EXPECT_TRUE(plc_authorized) << "the PLC owns one of the burst's faults"; +} + +TEST_F(BulkDataSourceFiltersTest, ABurstRecordingIsRejectedWhenNoAttachedFaultIsInScope) { + App plc; + plc.id = "plc"; + plc.external = true; + App plc_line1; + plc_line1.id = "plc_line1"; + plc_line1.external = true; + + ThreadSafeEntityCache cache; + cache.update_all({}, {}, {plc, plc_line1}, {}); + + auto entity = make_entity_info(EntityType::APP, "plc", "", ""); + auto filters = handlers::detail::compute_bulkdata_source_filters(cache, entity); + std::set scope(filters.begin(), filters.end()); + + // The prefix-sibling rule has to survive the union: "plc" must not reach a + // burst owned entirely by "plc_line1", no matter how many faults it holds. + const std::vector foreign_burst = {nlohmann::json{{"reporting_sources", {"plc_line1"}}}, + nlohmann::json{{"reporting_sources", {"plc_line1/axis2"}}}}; + + const bool authorized = std::any_of(foreign_burst.begin(), foreign_burst.end(), [&](const nlohmann::json & f) { + return faults::fault_in_source_scope(f, scope); + }); + EXPECT_FALSE(authorized); + + // ...while a descendant of the entity's own scope still reaches it. + const std::vector own_burst = {nlohmann::json{{"reporting_sources", {"plc_line1"}}}, + nlohmann::json{{"reporting_sources", {"plc/axis1"}}}}; + EXPECT_TRUE(std::any_of(own_burst.begin(), own_burst.end(), [&](const nlohmann::json & f) { + return faults::fault_in_source_scope(f, scope); + })); +} + TEST_F(BulkDataSourceFiltersTest, DownloadOwnershipScopeRejectsPrefixSiblingApp) { App plc; plc.id = "plc"; diff --git a/src/ros2_medkit_integration_tests/CMakeLists.txt b/src/ros2_medkit_integration_tests/CMakeLists.txt index 669b2c199..b450f1863 100644 --- a/src/ros2_medkit_integration_tests/CMakeLists.txt +++ b/src/ros2_medkit_integration_tests/CMakeLists.txt @@ -248,8 +248,13 @@ if(BUILD_TESTING) # greenwave_monitor node through the same 10s launch-ordering delay plus # warmup as test_graph_provider_greenwave above, and their staleness/SSE # polling budgets are similarly generous - both widened to match. + # test_rosbag_history_download drives two full occurrences of one real fault - + # break the range, wait for the confirmation, wait for the post-roll to finalise + # into a listed descriptor, repair, acknowledge - and then downloads every bag. + # Two of those cycles do not fit the feature glob's 120s. set(_MEDKIT_TEST_TIMEOUT_OVERRIDES test_rosbag_boundary_download 180 + test_rosbag_history_download 180 test_graph_provider_greenwave 300 test_graph_provider_stale 300 test_graph_provider_sse 300) diff --git a/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py index c450ecba8..0c70027f5 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py @@ -135,10 +135,14 @@ def test_bulk_data_list_descriptors_structure(self): # Verify x-medkit extension self.assertIn('x-medkit', descriptor) x_medkit = descriptor['x-medkit'] - self.assertIn('fault_code', x_medkit) - # recording_id (bag directory basename) groups the descriptors of - # faults that share one recording. + # fault_codes, plural: one recording covers every fault of a burst, and + # the descriptor is per recording rather than per fault. + self.assertIn('fault_codes', x_medkit) + self.assertIsInstance(x_medkit['fault_codes'], list) + self.assertGreater(len(x_medkit['fault_codes']), 0) + # The descriptor id IS the recording id - that is what addresses the bag. self.assertIn('recording_id', x_medkit) + self.assertEqual(descriptor['id'], x_medkit['recording_id']) self.assertTrue( x_medkit['recording_id'].startswith('fault_'), f'recording_id should be the bag directory name, ' diff --git a/src/ros2_medkit_integration_tests/test/features/test_external_app_fault_rollup.test.py b/src/ros2_medkit_integration_tests/test/features/test_external_app_fault_rollup.test.py index cab6f43f0..8e9fd5815 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_external_app_fault_rollup.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_external_app_fault_rollup.test.py @@ -161,7 +161,9 @@ def test_external_app_bulk_data_uris_serve_the_captured_bag(self): regression: ``GET /areas/plc-cell/faults/`` advertises ``/areas/plc-cell/bulk-data/rosbags/``, which 404'd while the area resolved its bulk-data scope through the namespace path instead - of its hosted apps' reporting sources. + of its hosted apps' reporting sources. Since #620 the ```` in + those URLs is a recording id; the bare fault code still resolves + through the compatibility path and is checked here too. Runs before the rollup test (method order is alphabetical) and reports the same fault code; the report is idempotent for both tests. @@ -176,13 +178,24 @@ def test_external_app_bulk_data_uris_serve_the_captured_bag(self): self.wait_for_fault(f'/apps/{EXTERNAL_APP}', FAULT_CODE) bag_id = self.wait_for_fault_with_rosbag(f'/apps/{EXTERNAL_APP}') - self.assertEqual(bag_id, FAULT_CODE) + # A bag is addressed by its recording id, which names the fault it was + # captured for without being equal to it - one fault can hold several. + self.assertTrue( + bag_id.startswith(f'fault_{FAULT_CODE}_'), + f'unexpected recording id: {bag_id}') # App-level download: 200 + bytes. - app_uri = f'/apps/{EXTERNAL_APP}/bulk-data/rosbags/{FAULT_CODE}' + app_uri = f'/apps/{EXTERNAL_APP}/bulk-data/rosbags/{bag_id}' resp = self.get_raw(app_uri) self.assertGreater(len(resp.content), 0, f'{app_uri} returned no bytes') + # The pre-#620 address - the bare fault code - still serves the same + # bag, so anything built against the documented URL keeps working. + legacy_uri = f'/apps/{EXTERNAL_APP}/bulk-data/rosbags/{FAULT_CODE}' + legacy = self.get_raw(legacy_uri) + self.assertEqual(legacy.content, resp.content, + f'{legacy_uri} no longer serves the same bytes') + # Area rollup advertises an area-scoped URI for the same bag. detail = self.wait_for_fault_detail( f'/areas/{HOST_AREA}', snapshot_types={'rosbag'}) @@ -194,19 +207,19 @@ def test_external_app_bulk_data_uris_serve_the_captured_bag(self): rosbag_snaps, 'area fault detail lost the rosbag snapshot') area_uri = rosbag_snaps[0].get('bulk_data_uri') self.assertEqual( - area_uri, f'/areas/{HOST_AREA}/bulk-data/rosbags/{FAULT_CODE}') + area_uri, f'/areas/{HOST_AREA}/bulk-data/rosbags/{bag_id}') # The area listing shows the bag and the advertised URI downloads. listing = self.get_json(f'/areas/{HOST_AREA}/bulk-data/rosbags') listed_ids = [item.get('id') for item in listing.get('items', [])] - self.assertIn(FAULT_CODE, listed_ids) + self.assertIn(bag_id, listed_ids) resp = self.get_raw(area_uri) self.assertGreater(len(resp.content), 0, f'{area_uri} returned no bytes') # Function rollup resolves the same scope. listing = self.get_json(f'/functions/{HOST_FUNCTION}/bulk-data/rosbags') listed_ids = [item.get('id') for item in listing.get('items', [])] - self.assertIn(FAULT_CODE, listed_ids) + self.assertIn(bag_id, listed_ids) def test_external_app_fault_appears_on_every_rollup(self): """The external app's fault surfaces on app, component, function, area. diff --git a/src/ros2_medkit_integration_tests/test/features/test_rosbag_boundary_download.test.py b/src/ros2_medkit_integration_tests/test/features/test_rosbag_boundary_download.test.py index 1a13ece2b..d2f08f1d8 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_rosbag_boundary_download.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_rosbag_boundary_download.test.py @@ -165,6 +165,10 @@ def _publish_for(self, duration_sec, rate_hz=20.0): def _wait_for_bag_descriptor(self, fault_code, timeout=20.0): """Poll the bulk-data rosbag listing until *fault_code* has an item. + A descriptor is one recording, not one fault: the item is found through + ``x-medkit.fault_codes``, which lists every fault the bag covers. Its + ``id`` is the recording id and is deliberately not the fault code. + Returns ``None`` on timeout rather than failing, so each call site can say what its own missing descriptor means. """ @@ -172,7 +176,7 @@ def _wait_for_bag_descriptor(self, fault_code, timeout=20.0): while time.time() < deadline: listing = self.get_json(f'{APP_ENDPOINT}/bulk-data/rosbags') for item in listing.get('items', []): - if item.get('id') == fault_code: + if fault_code in item.get('x-medkit', {}).get('fault_codes', []): return item time.sleep(0.5) return None @@ -200,7 +204,8 @@ def test_01_boundary_fault_bag_via_gateway(self, proc_output): # That recording is a zero-message post-fault-only bag: nothing has ever # been published on the only captured topic. The config docs promise such # a bag is still listed and downloadable, so pin it over HTTP rather than - # only at the fault manager's service. + # only at the fault manager's service. Addressed by fault code, which + # since #620 resolves through the compatibility path. empty_bag = self.get_raw( f'{APP_ENDPOINT}/bulk-data/rosbags/{LIDAR_OWN_FAULT}', timeout=10) self.assertEqual(empty_bag.content[:5], b'\x89MCAP', @@ -259,14 +264,16 @@ def test_01_boundary_fault_bag_via_gateway(self, proc_output): ) self.assertIsNotNone( detail, "fault B's detail must list a rosbag snapshot") + # The detail advertises the recording, not the fault code: a fault can + # hold several recordings and only the id distinguishes them. self.assertTrue( detail['bulk_data_uri'].endswith( - f'/bulk-data/rosbags/{FAULT_B}'), + f'/bulk-data/rosbags/{x_medkit.get("recording_id")}'), f'unexpected bulk_data_uri: {detail["bulk_data_uri"]}') # --- Download through the gateway --- response = self.get_raw( - f'{APP_ENDPOINT}/bulk-data/rosbags/{FAULT_B}', + f'{APP_ENDPOINT}/bulk-data/rosbags/{x_medkit.get("recording_id")}', timeout=10, ) self.assertIn('application/x-mcap', diff --git a/src/ros2_medkit_integration_tests/test/features/test_rosbag_history_download.test.py b/src/ros2_medkit_integration_tests/test/features/test_rosbag_history_download.test.py new file mode 100644 index 000000000..f124f6719 --- /dev/null +++ b/src/ros2_medkit_integration_tests/test/features/test_rosbag_history_download.test.py @@ -0,0 +1,335 @@ +#!/usr/bin/env python3 +# Copyright 2026 mfaferek93 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""End-to-end HTTP surface of a fault that keeps several recordings (#620). + +An intermittent fault used to leave exactly one black box no matter how often +it came back: the newest recording overwrote the previous one, so the recording +of the occurrence an engineer actually wanted was already gone by the time they +looked. This suite drives that scenario through the whole stack and asserts the +evidence is all still reachable over SOVD. + +The fault is a real one, detected by the node itself. ``lidar_sensor`` checks +``min_range >= max_range`` on every parameter change and on a 2s timer, and +reports ``LIDAR_RANGE_INVALID`` on its own; the test only moves the parameters +that make the condition true, the way a misconfiguration would. Nothing here +calls ``ReportFault`` by hand, so the recordings are triggered by the same code +path a deployed sensor would take. + +Every assertion goes through the gateway's HTTP API rather than the fault +manager's ROS services - the bulk-data descriptor list, the fault detail's +snapshot URIs and the binary downloads are what a diagnostic client actually +consumes, and the recording-id addressing this change introduces only exists +there. +""" + +import os +import tempfile +import time +import unittest + +import launch_testing +from rcl_interfaces.srv import SetParameters +import rclpy +from rclpy.node import Node +from rclpy.parameter import Parameter + +from ros2_medkit_test_utils.constants import ALLOWED_EXIT_CODES +from ros2_medkit_test_utils.gateway_test_case import GatewayTestCase +from ros2_medkit_test_utils.launch_helpers import create_test_launch + + +APP_ENDPOINT = '/apps/lidar_sensor' +LIDAR_NAMESPACE = '/perception/lidar' +FAULT_CODE = 'LIDAR_RANGE_INVALID' + +# The recording the fault manager keeps per fault code. 3 leaves headroom above +# the two occurrences driven below, so a failure means "history was lost", not +# "the cap trimmed it". +MAX_BAGS_PER_FAULT = 3 + +# The lidar's own scan output. Recording a topic the node really publishes is +# what makes the downloaded bags non-trivial. +CAPTURED_TOPIC = f'{LIDAR_NAMESPACE}/scan' + +ROSBAG_STORAGE_PATH = tempfile.mkdtemp(prefix='rosbag_e2e_history_') + + +def generate_test_description(): + return create_test_launch( + demo_nodes=['lidar_sensor'], + # Start the lidar healthy on the range check. The test drives the fault + # itself, so a node already faulting at startup would blur which + # occurrence produced which recording. + lidar_faulty=False, + fault_manager=True, + fault_manager_params={ + 'confirmation_threshold': -1, # Single report confirms immediately + 'snapshots.rosbag.duration_sec': 2.0, + 'snapshots.rosbag.duration_after_sec': 0.5, + 'snapshots.rosbag.include_topics': [CAPTURED_TOPIC], + 'snapshots.rosbag.format': 'mcap', + 'snapshots.rosbag.storage_path': ROSBAG_STORAGE_PATH, + # The feature under test. + 'snapshots.rosbag.max_bags_per_fault': MAX_BAGS_PER_FAULT, + # LEFT ON, which is the shipped default and the point: acknowledging + # used to delete every recording of the fault, so the history the cap + # above was raised to collect was wiped by the first acknowledgement. + 'snapshots.rosbag.auto_cleanup': True, + + }, + ) + + +class TestRosbagHistoryDownload(GatewayTestCase): + """Two occurrences of one fault, both recordings reachable over SOVD.""" + + MIN_EXPECTED_APPS = 1 + REQUIRED_APPS = {'lidar_sensor'} + + @classmethod + def setUpClass(cls): + super().setUpClass() + rclpy.init() + cls._param_node = Node('rosbag_history_param_client') + # Direct SetParameters client rather than AsyncParameterClient, which is + # Jazzy+ only - same choice as test_graph_provider_stale, so this suite + # also loads on Humble. + cls._param_client = cls._param_node.create_client( + SetParameters, f'{LIDAR_NAMESPACE}/lidar_sensor/set_parameters', + ) + assert cls._param_client.wait_for_service(timeout_sec=20.0), \ + 'lidar_sensor parameter services not available' + + @classmethod + def tearDownClass(cls): + cls._param_node.destroy_node() + rclpy.shutdown() + super().tearDownClass() + + # ------------------------------------------------------------------ + # Driving the real fault + # ------------------------------------------------------------------ + + def _set_range(self, min_range, max_range): + """Live-set the lidar's range window, asserting the node accepted it.""" + request = SetParameters.Request() + request.parameters = [ + Parameter('min_range', Parameter.Type.DOUBLE, min_range).to_parameter_msg(), + Parameter('max_range', Parameter.Type.DOUBLE, max_range).to_parameter_msg(), + ] + future = self._param_client.call_async(request) + rclpy.spin_until_future_complete(self._param_node, future, timeout_sec=10.0) + result = future.result() + self.assertIsNotNone(result, 'set_parameters call to lidar_sensor timed out') + for outcome in result.results: + self.assertTrue(outcome.successful, + f'lidar_sensor rejected the range change: {outcome.reason}') + + def _break_the_range(self): + """Make min_range >= max_range, which the node reports on its own.""" + self._set_range(10.0, 5.0) + + def _repair_the_range(self): + """Back to a valid window so the node stops reporting.""" + self._set_range(0.1, 30.0) + + def _clear_fault(self): + """Acknowledge the fault over SOVD, the way a technician would.""" + # The ROS-backed clear path answers 204 No Content; the 200 variant of + # this route only exists for plugin-provided faults. + self.delete_request(f'{APP_ENDPOINT}/faults/{FAULT_CODE}', expected_status=204) + + # ------------------------------------------------------------------ + # Reading the evidence back over HTTP + # ------------------------------------------------------------------ + + def _recordings_of_the_fault(self): + """Descriptor ids the bulk-data listing attributes to our fault code.""" + listing = self.get_json(f'{APP_ENDPOINT}/bulk-data/rosbags') + return [ + item['id'] for item in listing.get('items', []) + if FAULT_CODE in item.get('x-medkit', {}).get('fault_codes', []) + ] + + def _wait_for_recording_count(self, expected, *, timeout=25.0): + """Poll the listing until the fault has *expected* recordings.""" + deadline = time.time() + timeout + ids = [] + while time.time() < deadline: + ids = self._recordings_of_the_fault() + if len(ids) >= expected: + return ids + time.sleep(0.5) + return ids + + def _record_one_occurrence(self, expected_total): + """Break the range, wait for the new bag, repair and acknowledge.""" + self._break_the_range() + self.wait_for_fault(APP_ENDPOINT, FAULT_CODE) + + ids = self._wait_for_recording_count(expected_total) + self.assertEqual( + len(ids), expected_total, + f'expected {expected_total} recording(s) for {FAULT_CODE} after this ' + f'occurrence, got {sorted(ids)}') + + # Stop the condition, then acknowledge, so the next occurrence is a + # genuine new confirmation rather than a continuation of this one. + self._repair_the_range() + self._clear_fault() + return ids + + # ------------------------------------------------------------------ + # Test + # ------------------------------------------------------------------ + + def test_01_two_occurrences_leave_two_downloadable_recordings(self): + """The reported bug, end to end over SOVD. + + @verifies REQ_INTEROP_072 + """ + first_ids = self._record_one_occurrence(1) + all_ids = self._record_one_occurrence(2) + + self.assertEqual(len(all_ids), 2, + 'the second occurrence overwrote the first recording') + self.assertEqual(len(set(all_ids)), 2, + 'both occurrences share one recording id, so only one ' + 'of them is addressable') + self.assertIn(first_ids[0], all_ids, + "the first occurrence's recording is gone - this is #620") + + # --- Every recording downloads as a real bag --- + payloads = {} + for recording_id in all_ids: + response = self.get_raw( + f'{APP_ENDPOINT}/bulk-data/rosbags/{recording_id}', timeout=15) + self.assertIn('application/x-mcap', + response.headers.get('Content-Type', '')) + self.assertEqual(response.content[:5], b'\x89MCAP', + f'{recording_id} did not download as a valid mcap') + payloads[recording_id] = response.content + + # Distinct bytes, not the same bag served under two names. Without this + # the test would pass on a build that resolved both ids to one bag. + self.assertEqual(len(set(payloads.values())), 2, + 'both recording ids served identical bytes') + + # --- The fault itself advertises both --- + detail = self.get_json(f'{APP_ENDPOINT}/faults/{FAULT_CODE}') + rosbag_snaps = [ + s for s in detail.get('environment_data', {}).get('snapshots', []) + if s.get('type') == 'rosbag' + ] + self.assertEqual(len(rosbag_snaps), 2, + 'the fault detail lists a different number of rosbag ' + f'snapshots than the {len(all_ids)} recordings it has') + # Each recording carries its own capture time. The fault manager has + # always stamped one, but the transport dropped it on the rosbag branch, + # so every bag reached a client with none and the UIs rendered "N/A". + # With one recording per fault that was cosmetic; with several it removes + # the only field an engineer can use to pick an occurrence. + captured = [s.get('x-medkit', {}).get('captured_at') for s in rosbag_snaps] + for value in captured: + self.assertIsNotNone(value, 'a recording reached the client with no capture time') + self.assertRegex(value, r'^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$') + self.assertEqual(len(set(captured)), 2, + 'both recordings report the same instant, so neither can be placed') + + advertised = {s['bulk_data_uri'] for s in rosbag_snaps} + self.assertEqual( + advertised, + {f'{APP_ENDPOINT}/bulk-data/rosbags/{rid}' for rid in all_ids}, + 'the URIs the fault advertises do not match its recordings') + + # Every advertised URI is one a client can actually follow. + for uri in advertised: + self.assertEqual(self.get_raw(uri, timeout=15).content[:5], b'\x89MCAP') + + def test_02_the_fault_code_url_still_serves_the_newest_recording(self): + """The compatibility window, over HTTP. + + The repo's own docs and the SSE payload tell clients to build + ``/{entity}/bulk-data/rosbags/{fault_code}``. That address has to keep + working, and to mean what it always meant: this fault's latest bag. + + @verifies REQ_INTEROP_072 + """ + ids = self._recordings_of_the_fault() + self.assertEqual(len(ids), 2, 'test_01 must run first (alphabetical order)') + + legacy = self.get_raw( + f'{APP_ENDPOINT}/bulk-data/rosbags/{FAULT_CODE}', timeout=15) + self.assertEqual(legacy.content[:5], b'\x89MCAP', + 'the pre-#620 fault-code URL stopped serving a bag') + + # "Newest" is not "either one": the detail lists snapshots newest first, + # so the legacy URL must serve exactly the first of them. + detail = self.get_json(f'{APP_ENDPOINT}/faults/{FAULT_CODE}') + newest_uri = next( + s['bulk_data_uri'] + for s in detail['environment_data']['snapshots'] + if s.get('type') == 'rosbag' + ) + self.assertEqual( + legacy.content, self.get_raw(newest_uri, timeout=15).content, + 'the fault-code URL served something other than the newest recording') + + def test_03_a_shared_burst_recording_is_listed_once(self): + """One descriptor per recording, not per attached fault. + + The lidar's calibration fault confirms at startup and its own recording + exists alongside the range fault's. Whatever the burst structure turned + out to be, no bag may appear twice in the listing: a repeated id would + report the same bytes as several items and inflate the storage the + operator sees. + + @verifies REQ_INTEROP_072 + """ + listing = self.get_json(f'{APP_ENDPOINT}/bulk-data/rosbags') + items = listing.get('items', []) + self.assertGreater(len(items), 0, 'no rosbag descriptors at all') + + ids = [item['id'] for item in items] + self.assertEqual(len(ids), len(set(ids)), + f'the listing repeats a recording id: {sorted(ids)}') + + for item in items: + x_medkit = item.get('x-medkit', {}) + # The descriptor id IS the recording id, and the faults it covers are + # a list because a burst shares one bag. + self.assertEqual(item['id'], x_medkit.get('recording_id')) + self.assertIsInstance(x_medkit.get('fault_codes'), list) + self.assertGreater(len(x_medkit['fault_codes']), 0) + self.assertEqual( + len(x_medkit['fault_codes']), len(set(x_medkit['fault_codes'])), + f'{item["id"]} lists a fault code twice') + + +@launch_testing.post_shutdown_test() +class TestShutdown(unittest.TestCase): + """Post-shutdown checks.""" + + def test_exit_codes(self, proc_info): + """Gateway and fault manager exit cleanly.""" + launch_testing.asserts.assertExitCodes( + proc_info, allowable_exit_codes=ALLOWED_EXIT_CODES) + + def test_cleanup_temp_directory(self): + """Remove this run's bag storage.""" + import shutil + if os.path.exists(ROSBAG_STORAGE_PATH): + shutil.rmtree(ROSBAG_STORAGE_PATH, ignore_errors=True) diff --git a/src/ros2_medkit_msgs/CHANGELOG.rst b/src/ros2_medkit_msgs/CHANGELOG.rst index 9b74117b6..1d45c10b7 100644 --- a/src/ros2_medkit_msgs/CHANGELOG.rst +++ b/src/ros2_medkit_msgs/CHANGELOG.rst @@ -2,6 +2,10 @@ Changelog for package ros2_medkit_msgs ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +Forthcoming +----------- +* ``GetRosbag.srv`` gains a ``recording_id`` request field (tried before ``fault_code`` and falling back to it when it names no recording, so a caller holding one identifier that may be either can set both; ``fault_code`` keeps its meaning of "the newest recording of this fault") and ``recording_id`` / ``fault_codes[]`` response fields. ``ListRosbags.srv`` gains a parallel ``recording_ids[]``. ``Snapshot.msg``'s ``bulk_data_id`` now carries a recording id for rosbag snapshots rather than the fault code; the field type is unchanged. Additive, but the service type hashes change, so the gateway and the fault manager must be deployed together (`#620 `_) + 0.6.0 (2026-06-22) ------------------ * No functional changes; version bump for the coordinated 0.6.0 release. diff --git a/src/ros2_medkit_msgs/msg/Snapshot.msg b/src/ros2_medkit_msgs/msg/Snapshot.msg index cb815df62..cb585dae7 100644 --- a/src/ros2_medkit_msgs/msg/Snapshot.msg +++ b/src/ros2_medkit_msgs/msg/Snapshot.msg @@ -37,7 +37,9 @@ string message_type # --- Rosbag fields (empty for freeze_frame type) --- -# Identifier for bulk-data download (set to fault_code) +# Identifier for bulk-data download. For a rosbag this is the RECORDING id (the +# bag directory basename), not the fault code: one fault can hold several +# recordings and each needs its own address. string bulk_data_id # File size in bytes diff --git a/src/ros2_medkit_msgs/srv/GetRosbag.srv b/src/ros2_medkit_msgs/srv/GetRosbag.srv index 96aa33a2f..d8edf0e8e 100644 --- a/src/ros2_medkit_msgs/srv/GetRosbag.srv +++ b/src/ros2_medkit_msgs/srv/GetRosbag.srv @@ -22,6 +22,13 @@ # The fault_code to get rosbag for. string fault_code + +# Address one specific recording instead. Tried before fault_code, and when it +# names no recording the request falls back to fault_code - so a caller holding a +# single identifier that may be either (a URL segment, say) can set both to it. +# fault_code alone keeps its historical meaning, "the newest recording of this +# fault". +string recording_id --- # Response fields @@ -32,6 +39,14 @@ bool success # Empty if success is false. string file_path +# Identity of the recording that was served (its bag directory basename). Use this +# as the bulk-data id. +string recording_id + +# Every fault this recording covers - a burst of correlated faults shares one bag. +# Callers authorize a download against this set, not against a single code. +string[] fault_codes + # Storage format: "sqlite3" or "mcap" string format diff --git a/src/ros2_medkit_msgs/srv/ListRosbags.srv b/src/ros2_medkit_msgs/srv/ListRosbags.srv index 494d5ed6a..ed5345404 100644 --- a/src/ros2_medkit_msgs/srv/ListRosbags.srv +++ b/src/ros2_medkit_msgs/srv/ListRosbags.srv @@ -32,6 +32,10 @@ bool success string[] fault_codes # Paths to the rosbag files/directories. +# Parallel to fault_codes. Rows of one burst repeat the same recording id, which is +# how a client groups the entries that serve the same bytes. +string[] recording_ids + string[] file_paths # Storage formats: "sqlite3" or "mcap"