fix(fault_manager): address review on rosbag crash-safety

Inject the storage probe as a seam and add direct tests for the fallback
and self-disable branches (mcap is in CI so the real probe never reached
them). Surface the probe failure reason + install hint at WARN, with a
distinct message when the always-shipped sqlite3 baseline itself fails.
Make mcap a test-only dependency and document the automatic fallback;
default bulk-data mimetype to sqlite3 for bags without a format field.

Author: mfaferek93

docs/tutorials/snapshots.rst

@@ -454,7 +454,16 @@ Saves resources but may miss context if fault confirms before buffer fills.
 .. note::
 
    The ``"mcap"`` format requires ``rosbag2_storage_mcap`` to be installed.
-   If not available, use ``"sqlite3"`` (default).
+   ``"sqlite3"`` (the default) is always shipped with rosbag2 and needs no extra
+   package.
+
+   You do not have to switch formats manually: if a configured backend's plugin
+   is unavailable at startup, the FaultManager logs a warning naming the missing
+   package and automatically falls back to ``"sqlite3"`` for black-box capture.
+   If no storage backend is usable at all, rosbag capture self-disables (the node
+   keeps running and freeze-frame snapshots are unaffected) instead of crashing.
+
+   To use mcap (e.g. for Foxglove), install the plugin:
 
    .. code-block:: bash
 

src/ros2_medkit_fault_manager/include/ros2_medkit_fault_manager/rosbag_capture.hpp

@@ -16,9 +16,11 @@
 
 #include <atomic>
 #include <deque>
+#include <functional>
 #include <map>
 #include <memory>
 #include <mutex>
+#include <optional>
 #include <set>
 #include <string>
 #include <vector>
@@ -53,13 +55,19 @@ struct BufferedMessage {
 /// - on_fault_cleared() deletes bag file if auto_cleanup enabled
 class RosbagCapture {
  public:
+  /// Probe a rosbag2 storage backend. Returns std::nullopt when the backend is
+  /// usable, or the failure reason (never throws). Injectable so tests can force a
+  /// backend unavailable without depending on which plugins CI happens to install.
+  using StorageProbeFn = std::function<std::optional<std::string>(const std::string & format)>;
+
   /// Create rosbag capture
   /// @param node ROS 2 node for creating subscriptions and timers
   /// @param storage Fault storage for persisting bag file metadata
   /// @param config Rosbag configuration
   /// @param snapshot_config Snapshot configuration (for topic resolution when topics="config")
+  /// @param storage_probe Optional storage-backend probe override (default: real probe)
   RosbagCapture(rclcpp::Node * node, FaultStorage * storage, const RosbagConfig & config,
-                const SnapshotConfig & snapshot_config);
+                const SnapshotConfig & snapshot_config, StorageProbeFn storage_probe = {});
 
   ~RosbagCapture();
 
@@ -143,10 +151,13 @@ class RosbagCapture {
   /// Timer callback for retrying topic discovery
   void discovery_retry_callback();
 
-  /// Probe whether a rosbag2 storage plugin is available by opening a throwaway
-  /// bag. Returns false (never throws) when the plugin is not installed, so the
-  /// caller can degrade gracefully instead of terminating the node.
-  bool storage_format_available(const std::string & format) const;
+  /// Default storage probe: opens a throwaway bag for @p format. Returns
+  /// std::nullopt when usable, or the failure reason (never throws), so the caller
+  /// can degrade gracefully instead of terminating the node.
+  std::optional<std::string> default_storage_probe(const std::string & format) const;
+
+  /// Storage-backend probe (the default real probe, or a test override).
+  StorageProbeFn storage_probe_;
 
   rclcpp::Node * node_;
   FaultStorage * storage_;

src/ros2_medkit_fault_manager/package.xml

@@ -18,10 +18,12 @@
   <depend>nlohmann-json-dev</depend>
   <depend>rosbag2_cpp</depend>
   <depend>rosbag2_storage</depend>
-  <depend>rosbag2_storage_mcap</depend>
 
   <test_depend>ament_lint_auto</test_depend>
   <test_depend>ament_lint_common</test_depend>
+  <!-- mcap is an opt-in storage backend (sqlite3 is the always-shipped default with
+       a runtime fallback); only the rosbag integration test needs the plugin present -->
+  <test_depend>rosbag2_storage_mcap</test_depend>
   <test_depend>ament_cmake_clang_format</test_depend>
   <test_depend>ament_cmake_clang_tidy</test_depend>
   <test_depend>ament_cmake_gtest</test_depend>

src/ros2_medkit_fault_manager/src/rosbag_capture.cpp

@@ -18,18 +18,39 @@
 #include <unistd.h>
 
 #include <algorithm>
+#include <cstdlib>
 #include <filesystem>
+#include <optional>
 #include <rosbag2_cpp/writer.hpp>
 #include <rosbag2_storage/storage_options.hpp>
 #include <set>
 #include <sstream>
+#include <utility>
 
 #include "ros2_medkit_fault_manager/time_utils.hpp"
 
 namespace ros2_medkit_fault_manager {
 
 namespace {
 
+/// Actionable install hint for an unavailable storage backend.
+std::string storage_plugin_hint(const std::string & format) {
+  if (format == "mcap") {
+    const char * distro = std::getenv("ROS_DISTRO");
+    const std::string d = (distro && *distro) ? distro : "$ROS_DISTRO";
+    return "install ros-" + d + "-rosbag2-storage-mcap";
+  }
+  return "check the rosbag2 storage plugin installation";
+}
+
+/// Bound the probe reason so a verbose pluginlib error does not flood the log.
+std::string truncate_reason(const std::string & reason, size_t max_len = 200) {
+  if (reason.size() <= max_len) {
+    return reason;
+  }
+  return reason.substr(0, max_len) + "...";
+}
+
 /// Custom deleter for rcutils_uint8_array_t that calls rcutils_uint8_array_fini
 struct Uint8ArrayDeleter {
   void operator()(rcutils_uint8_array_t * array) const {
@@ -85,7 +106,7 @@ std::shared_ptr<rosbag2_storage::SerializedBagMessage> create_bag_message(const
 }  // namespace
 
 RosbagCapture::RosbagCapture(rclcpp::Node * node, FaultStorage * storage, const RosbagConfig & config,
-                             const SnapshotConfig & snapshot_config)
+                             const SnapshotConfig & snapshot_config, StorageProbeFn storage_probe)
   : node_(node), storage_(storage), config_(config), snapshot_config_(snapshot_config) {
   if (!node_) {
     throw std::invalid_argument("RosbagCapture requires a valid node pointer");
@@ -94,6 +115,10 @@ RosbagCapture::RosbagCapture(rclcpp::Node * node, FaultStorage * storage, const
     throw std::invalid_argument("RosbagCapture requires a valid storage pointer");
   }
 
+  storage_probe_ = storage_probe ? std::move(storage_probe) : [this](const std::string & f) {
+    return default_storage_probe(f);
+  };
+
   if (!config_.enabled) {
     RCLCPP_INFO(node_->get_logger(), "RosbagCapture disabled");
     return;
@@ -108,21 +133,34 @@ RosbagCapture::RosbagCapture(rclcpp::Node * node, FaultStorage * storage, const
     RCLCPP_WARN(node_->get_logger(), "Unknown rosbag storage format '%s'; using 'sqlite3'", config_.format.c_str());
     config_.format = "sqlite3";
   }
-  if (!storage_format_available(config_.format)) {
-    if (config_.format != "sqlite3" && storage_format_available("sqlite3")) {
+
+  if (auto probe_err = storage_probe_(config_.format)) {
+    const std::string reason = truncate_reason(*probe_err);
+    if (config_.format == "sqlite3") {
+      // The always-shipped baseline failed: an environment-level problem (broken
+      // rosbag2 base install / disk / permissions), not a missing optional plugin.
       RCLCPP_WARN(node_->get_logger(),
-                  "Rosbag storage format '%s' is unavailable (see debug log for the probe error); falling back "
-                  "to 'sqlite3' for black-box capture",
-                  config_.format.c_str());
-      config_.format = "sqlite3";
-    } else {
+                  "sqlite3 rosbag storage is unavailable (%s); the rosbag2 base install may be broken. "
+                  "Black-box rosbag capture disabled (freeze-frame snapshots still work)",
+                  reason.c_str());
+      config_.enabled = false;
+      return;
+    }
+    if (auto sqlite_err = storage_probe_("sqlite3")) {
       RCLCPP_WARN(node_->get_logger(),
-                  "No usable rosbag storage backend ('%s' and 'sqlite3' both unavailable); black-box rosbag "
-                  "capture disabled (freeze-frame snapshots still work)",
-                  config_.format.c_str());
+                  "No usable rosbag storage backend: '%s' (%s) and sqlite3 (%s) both failed to load; black-box "
+                  "rosbag capture disabled (freeze-frame snapshots still work)",
+                  config_.format.c_str(), reason.c_str(), truncate_reason(*sqlite_err).c_str());
       config_.enabled = false;
       return;
     }
+    // Explicitly-configured (non-default) format unavailable: name the fix so an
+    // operator who chose e.g. mcap for Foxglove is not silently downgraded.
+    RCLCPP_WARN(node_->get_logger(),
+                "Rosbag storage format '%s' is unavailable (%s); %s. Falling back to 'sqlite3' for black-box "
+                "capture (sqlite3 bags are not directly Foxglove-readable)",
+                config_.format.c_str(), reason.c_str(), storage_plugin_hint(config_.format).c_str());
+    config_.format = "sqlite3";
   }
 
   RCLCPP_INFO(node_->get_logger(), "RosbagCapture initialized (duration=%.1fs, after=%.1fs, lazy_start=%s, format=%s)",
@@ -721,30 +759,28 @@ void RosbagCapture::post_fault_timer_callback() {
   current_bag_path_.clear();
 }
 
-bool RosbagCapture::storage_format_available(const std::string & format) const {
-  // Probe the plugin by opening a throwaway bag. Returns false (never throws)
-  // when the plugin is missing, so the caller can fall back instead of crashing.
+std::optional<std::string> RosbagCapture::default_storage_probe(const std::string & format) const {
+  // Probe the plugin by opening a throwaway bag. Returns the failure reason (never
+  // throws) when the plugin is missing or unusable, so the caller can fall back or
+  // self-disable instead of crashing.
   const std::string test_path =
       std::filesystem::temp_directory_path().string() + "/.rosbag_format_test_" + std::to_string(getpid());
 
-  bool ok = false;
+  std::optional<std::string> error;
   try {
     rosbag2_cpp::Writer writer;
     rosbag2_storage::StorageOptions opts;
     opts.uri = test_path;
     opts.storage_id = format;
     writer.open(opts);
-    ok = true;
   } catch (const std::exception & e) {
-    // Keep the reason (plugin missing vs. I/O / permission) for diagnosis; the
-    // probe itself stays non-fatal.
-    RCLCPP_DEBUG(node_->get_logger(), "Rosbag storage format '%s' probe failed: %s", format.c_str(), e.what());
-    ok = false;
+    // Keep the reason (plugin missing vs. I/O / permission) for the caller's log.
+    error = e.what();
   }
 
   std::error_code ec;
   std::filesystem::remove_all(test_path, ec);
-  return ok;
+  return error;
 }
 
 }  // namespace ros2_medkit_fault_manager

src/ros2_medkit_fault_manager/test/test_rosbag_capture.cpp

@@ -17,6 +17,7 @@
 #include <chrono>
 #include <filesystem>
 #include <memory>
+#include <optional>
 #include <string>
 #include <thread>
 
@@ -122,6 +123,64 @@ TEST_F(RosbagCaptureTest, ConstructorFallsBackOnUnknownFormat) {
   EXPECT_EQ(rb->config().format, "sqlite3");
 }
 
+// The crash-safety branches below force the storage probe via an injected double,
+// because mcap is installed in CI so the real probe cannot reach them there.
+
+// @verifies REQ_INTEROP_088
+TEST_F(RosbagCaptureTest, ConfiguredFormatUnavailableFallsBackToSqlite3) {
+  // A known format (mcap) whose plugin is unavailable degrades to sqlite3 and
+  // capture stays enabled - the exact scenario the crash-safety change exists for.
+  auto rosbag_config = create_rosbag_config();
+  rosbag_config.format = "mcap";
+  auto snapshot_config = create_snapshot_config();
+  RosbagCapture::StorageProbeFn probe = [](const std::string & f) -> std::optional<std::string> {
+    if (f == "mcap") {
+      return std::string("simulated: mcap plugin not found");
+    }
+    return std::nullopt;  // sqlite3 usable
+  };
+  std::shared_ptr<RosbagCapture> rb;
+  EXPECT_NO_THROW(
+      rb = std::make_shared<RosbagCapture>(node_.get(), storage_.get(), rosbag_config, snapshot_config, probe));
+  ASSERT_NE(rb, nullptr);
+  EXPECT_TRUE(rb->is_enabled());
+  EXPECT_EQ(rb->config().format, "sqlite3");
+}
+
+// @verifies REQ_INTEROP_088
+TEST_F(RosbagCaptureTest, NoUsableBackendDisablesCaptureWithoutCrashing) {
+  // When neither the configured format nor sqlite3 is usable, capture self-disables
+  // and the node keeps running (no throw out of the constructor).
+  auto rosbag_config = create_rosbag_config();
+  rosbag_config.format = "mcap";
+  auto snapshot_config = create_snapshot_config();
+  RosbagCapture::StorageProbeFn probe = [](const std::string &) -> std::optional<std::string> {
+    return std::string("simulated: backend unavailable");
+  };
+  std::shared_ptr<RosbagCapture> rb;
+  EXPECT_NO_THROW(
+      rb = std::make_shared<RosbagCapture>(node_.get(), storage_.get(), rosbag_config, snapshot_config, probe));
+  ASSERT_NE(rb, nullptr);
+  EXPECT_FALSE(rb->is_enabled());
+}
+
+// @verifies REQ_INTEROP_088
+TEST_F(RosbagCaptureTest, Sqlite3BaselineUnavailableDisablesCapture) {
+  // When the always-shipped sqlite3 baseline itself fails to load, capture
+  // self-disables rather than crashing the node.
+  auto rosbag_config = create_rosbag_config();
+  rosbag_config.format = "sqlite3";
+  auto snapshot_config = create_snapshot_config();
+  RosbagCapture::StorageProbeFn probe = [](const std::string &) -> std::optional<std::string> {
+    return std::string("simulated: rosbag2 base install broken");
+  };
+  std::shared_ptr<RosbagCapture> rb;
+  EXPECT_NO_THROW(
+      rb = std::make_shared<RosbagCapture>(node_.get(), storage_.get(), rosbag_config, snapshot_config, probe));
+  ASSERT_NE(rb, nullptr);
+  EXPECT_FALSE(rb->is_enabled());
+}
+
 // @verifies REQ_INTEROP_088
 TEST_F(RosbagCaptureTest, ConstructorAcceptsMcapFormat) {
   auto rosbag_config = create_rosbag_config();

src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp

@@ -227,7 +227,9 @@ BulkDataHandlers::list_descriptors(const http::TypedRequest & req) {
     dto::Collection<dto::BulkDataDescriptor> response;
     for (const auto & rosbag : all_rosbags) {
       std::string fault_code = rosbag.value("fault_code", "");
-      std::string format = rosbag.value("format", "mcap");
+      // Default to sqlite3 (the 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);
 
@@ -347,7 +349,9 @@ http::Result<http::BinaryResponse> BulkDataHandlers::download(const http::TypedR
     }
 
     std::string file_path = rosbag_result.data["file_path"].get<std::string>();
-    std::string format = rosbag_result.data.value("format", "mcap");
+    // Default to sqlite3 (the FaultManager default) for bags predating the 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;