Declare the rosbag storage plugins, default to mcap, and serialize the plugin loader

Dockerfile

@@ -80,6 +80,17 @@ COPY src/ros2_medkit_plugins/ ${COLCON_WS}/src/ros2_medkit_plugins/
 # test_depend and which rosdep tries to install even with BUILD_TESTING=OFF).
 # This was previously masked by Docker layer cache hits on CI; cold builds
 # always failed.
+#
+# rosbag2_storage_mcap stays skip-keyed here even though fault_manager now
+# exec_depends on it: this stage compiles with -DBUILD_TESTING=OFF against
+# rosbag2_cpp/rosbag2_storage (the plugin interface, already resolvable
+# without it) and never runs a bag, so it never needs the plugin itself, only
+# its interface. The key exists because the plugin package was not available
+# via rosdep on every distro this Dockerfile targets when it was added
+# (ce702f61); that constraint is about apt package availability per distro,
+# not about whether fault_manager declares the dependency, so becoming an
+# exec_depend does not remove the reason to skip it here. It is installed for
+# real in the runtime stage below, which is where it is actually loaded.
 RUN bash -c "source /opt/ros/${ROS_DISTRO}/setup.bash && \
     apt-get update && \
     rosdep update && \
@@ -104,10 +115,23 @@ ENV COLCON_WS=/home/medkit/ws
 # CycloneDDS RMW is bundled so the gateway can attach to a stack running
 # CycloneDDS (e.g. Autoware) without rebuilding. FastDDS stays the default;
 # switch with RMW_IMPLEMENTATION=rmw_cyclonedds_cpp at runtime.
+# rosbag2-storage-mcap and rosbag2-storage-default-plugins are the actual
+# rosbag2 storage plugins fault_manager loads at runtime through
+# rosbag2_storage's plugin interface: the former is always the mcap plugin,
+# the latter is the sqlite3 plugin itself on humble but, on jazzy (this
+# stage's default) and lyrical, a metapackage that pulls in both sqlite3 and
+# mcap - so it is not a clean sqlite3-only counterpart to the first package on
+# every distro this image targets, just the name that guarantees sqlite3 is
+# present. libsqlite3-0 alone is the C library, not a rosbag2 plugin, and this
+# stage never gets /opt/ros from the builder - only the colcon workspace
+# install/ - so without these two the image would advertise mcap-by-default
+# and silently have no working black-box storage backend at all.
 RUN apt-get update && apt-get install -y --no-install-recommends \
     ros-${ROS_DISTRO}-yaml-cpp-vendor \
     ros-${ROS_DISTRO}-example-interfaces \
     ros-${ROS_DISTRO}-rmw-cyclonedds-cpp \
+    ros-${ROS_DISTRO}-rosbag2-storage-mcap \
+    ros-${ROS_DISTRO}-rosbag2-storage-default-plugins \
     libsqlite3-0 \
     libsystemd0 \
     libssl3 \

QUALITY_DECLARATION.md

@@ -195,6 +195,8 @@ Linter tests are enforced in CI on every pull request.
 | `diagnostic_msgs` | Level 1 |
 | `rosbag2_cpp` | Level 3 |
 | `rosbag2_storage` | Level 3 |
+| `rosbag2_storage_mcap` | Level 3 |
+| `rosbag2_storage_default_plugins` | Level 3 |
 
 ### Optional Direct Runtime Non-ROS Dependencies [5.ii]
 

docs/config/fault-manager.rst

@@ -244,7 +244,7 @@ Capture continuous rosbag recordings around fault events.
            exclude_topics: []              # Topics to exclude
            exclude_sensor_topics: true     # Auto-exclude image/points/depth/compressed in broad modes
            lazy_start: false               # Start recording on first fault
-           format: "sqlite3"               # Storage format
+           format: "mcap"                  # Storage format (default: mcap)
            qos_match: true                 # Match each topic's publisher QoS
            storage_path: ""                # Custom storage path
            max_buffer_mb: 256              # Ring-buffer RAM cap
@@ -293,10 +293,6 @@ Capture continuous rosbag recordings around fault events.
    * - ``rosbag.exclude_topics``
      - ``[]``
      - Topics to drop from whatever the selection mode picked.
-   * - ``rosbag.format``
-     - ``sqlite3``
-     - Storage format handed to rosbag2. Use ``mcap`` where the reader expects
-       it.
    * - ``rosbag.storage_path``
      - ``""``
      - Directory the bag files are written to. Empty falls back to the system
@@ -306,6 +302,14 @@ Capture continuous rosbag recordings around fault events.
      - ``true``
      - Subscribe with each topic's publisher-offered QoS for faithful capture
        instead of forcing best-effort.
+   * - ``rosbag.format``
+     - ``mcap``
+     - Bag storage format: ``mcap`` (default; opens directly in Foxglove and
+       Lichtblick) or ``sqlite3`` (also what an unknown format string lands
+       on). Neither is privileged: whichever is configured, an unavailable
+       plugin falls back automatically to the other one, and capture disables
+       itself only if neither loads. Both plugins are runtime dependencies of
+       ``ros2_medkit_fault_manager``.
    * - ``rosbag.max_buffer_mb``
      - ``256``
      - Ring-buffer RAM cap; oldest buffered messages drop past it.
@@ -384,7 +388,7 @@ both stay - the sweep is about the whole store, not about this recording, and it
 runs again on the next capture.
 
 A post-fault-only bag on a quiet or heavily filtered system can contain zero
-messages. It still finalises normally on both ``sqlite3`` and ``mcap``, is listed
+messages. It still finalises normally on both ``mcap`` and ``sqlite3``, is listed
 by the bulk-data endpoints and can be downloaded; only its payload is empty.
 
 **What ``duration_sec`` on a stored bag means.** The value returned by

docs/tutorials/snapshots.rst

@@ -478,9 +478,13 @@ Rosbag Configuration Options
      - In-memory ring-buffer cap; oldest buffered messages drop once exceeded, so a
        broad subscribe set cannot grow memory without bound.
    * - ``snapshots.rosbag.format``
-     - ``"sqlite3"``
-     - Bag storage format: ``"sqlite3"`` (default, widely compatible) or
-       ``"mcap"`` (more efficient compression, requires plugin).
+     - ``"mcap"``
+     - Bag storage format: ``"mcap"`` (default; opens directly in Foxglove and
+       Lichtblick) or ``"sqlite3"`` (also what an unknown format string lands
+       on). Neither is privileged: whichever is configured, an unavailable
+       plugin falls back automatically to the other one, and capture disables
+       itself only if neither loads. Both plugins are runtime dependencies of
+       the package.
    * - ``snapshots.rosbag.storage_path``
      - ``""``
      - Directory for bag files. Empty string uses system temp directory
@@ -671,22 +675,19 @@ Saves resources but may miss context if fault confirms before buffer fills.
 
 .. note::
 
-   The ``"mcap"`` format requires ``rosbag2_storage_mcap`` to be installed.
-   ``"sqlite3"`` (the default) is always shipped with rosbag2 and needs no extra
-   package.
+   ``"mcap"`` (the default) opens directly in Foxglove and Lichtblick with no
+   conversion step. ``"sqlite3"`` is also what an unknown format string lands
+   on. Both ``rosbag2_storage_mcap`` and ``rosbag2_storage_default_plugins``
+   (which carries the sqlite3 plugin) are runtime dependencies of the package,
+   so both are installed alongside it, and neither is privileged over the
+   other.
 
    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
-
-      # Install MCAP support (optional)
-      sudo apt install ros-${ROS_DISTRO}-rosbag2-storage-mcap
+   is somehow still unavailable at startup, the FaultManager logs a warning
+   naming the missing package and automatically falls back to the other
+   backend for black-box capture. If neither storage backend is usable at all,
+   rosbag capture self-disables (the node keeps running and freeze-frame
+   snapshots are unaffected) instead of crashing.
 
 Downloading Rosbag Files
 ^^^^^^^^^^^^^^^^^^^^^^^^
@@ -758,7 +759,6 @@ For production use with conservative resource usage:
      duration_after_sec: 0.5
      topics: "config"           # Use same topics as JSON snapshots
      lazy_start: true           # Save resources until fault detected
-     format: "sqlite3"
      max_bag_size_mb: 25
      max_total_storage_mb: 200
      auto_cleanup: true
@@ -781,7 +781,6 @@ For development with maximum context:
      duration_after_sec: 2.0    # 2 seconds after
      topics: "config"
      lazy_start: false          # Always recording
-     format: "sqlite3"
      storage_path: "/var/log/ros2_medkit/rosbags"
      max_bag_size_mb: 100
      max_total_storage_mb: 1000

src/ros2_medkit_fault_manager/CMakeLists.txt

@@ -114,6 +114,14 @@ if(BUILD_TESTING)
 
   include(ROS2MedkitTestDomain)
 
+  # Manifest test: the storage plugins declared as exec/test dependencies.
+  # Creates no ROS node, so it needs no domain.
+  find_package(ament_cmake_pytest REQUIRED)
+  ament_add_pytest_test(test_rosbag_storage_dependency test/test_rosbag_storage_dependency.py
+    TIMEOUT 60)
+  set_tests_properties(test_rosbag_storage_dependency PROPERTIES LABELS "unit")
+  medkit_test_needs_no_domain(test_rosbag_storage_dependency)
+
   # Unit tests
   # Each GTest target that instantiates ROS 2 nodes takes a ROS_DOMAIN_ID from this
   # package's pool so it cannot interfere with launch_testing integration tests that run
@@ -171,6 +179,11 @@ if(BUILD_TESTING)
   medkit_add_launch_test(test_rosbag_integration test/test_rosbag_integration.test.py TIMEOUT 90
     LABELS "integration")
 
+  # Launches without snapshots.rosbag.format, so the parameter default (mcap)
+  # is what is under test rather than an operator's explicit choice.
+  medkit_add_launch_test(test_rosbag_default_format test/test_rosbag_default_format.test.py TIMEOUT 90
+    LABELS "integration")
+
   medkit_add_launch_test(test_entity_thresholds_integration test/test_entity_thresholds_integration.test.py
     TIMEOUT 60 LABELS "integration")
 

src/ros2_medkit_fault_manager/README.md

@@ -109,8 +109,25 @@ patterns:
     - /cmd_vel
 ```
 
+### Rosbag Capture
+
+Black-box rosbag capture (`snapshots.rosbag.*`) writes bags in `mcap` format by
+default, which Foxglove and Lichtblick open directly with no conversion step.
+`sqlite3` remains available through `snapshots.rosbag.format` and is also what an
+unknown format string lands on. Neither backend is privileged over the other:
+whichever is configured, if its plugin is unavailable at startup the FaultManager
+logs a warning naming the missing package and falls back to the other one
+automatically, disabling capture only if neither loads. Both `rosbag2_storage_mcap`
+and `rosbag2_storage_default_plugins` (which carries the sqlite3 plugin) are runtime
+dependencies of this package, so a normal install pulls in both backends. See
+`docs/config/fault-manager.rst` and `docs/tutorials/snapshots.rst` for the full
+parameter list and recording lifecycle.
+
 ### Storage Backends
 
+This is the persisted fault store (`storage_type`), separate from the rosbag storage
+format used by black-box capture (`snapshots.rosbag.format`, see Rosbag Capture above).
+
 **SQLite (default)**: Faults are persisted to disk and survive node restarts. Uses WAL mode for optimal performance.
 
 **Memory**: Faults are stored in memory only. Useful for testing or when persistence is not required.

src/ros2_medkit_fault_manager/config/snapshots.yaml

@@ -130,13 +130,18 @@ rosbag:
   # When false, ring buffer runs continuously from node startup
   lazy_start: false
 
-  # Bag file format (default: "sqlite3")
+  # Bag file format (default: "mcap")
   # Options:
-  #   "sqlite3" - Default; always shipped with rosbag2, zero extra setup
-  #   "mcap"    - Opt-in; efficient and Foxglove-readable, needs rosbag2_storage_mcap
-  # If the selected plugin is not installed, capture automatically falls back to
-  # sqlite3, and disables itself only if no backend is usable (never crashes).
-  format: "sqlite3"
+  #   "mcap"    - Default; opens directly in Foxglove and Lichtblick
+  #   "sqlite3" - Also what an unknown format string lands on
+  # Neither backend is privileged over the other: whichever is configured, if
+  # its plugin is unavailable at startup the FaultManager logs a warning
+  # naming the missing package and falls back to the other one automatically,
+  # disabling capture only if neither loads (never crashes). Both
+  # rosbag2_storage_mcap and rosbag2_storage_default_plugins (the sqlite3
+  # plugin) are runtime dependencies of this package, so a normal install
+  # pulls in both backends.
+  format: "mcap"
 
   # Storage path for bag files (default: "" = system temp directory)
   # Empty string uses /tmp/rosbag_snapshots/

src/ros2_medkit_fault_manager/design/index.rst

@@ -336,13 +336,67 @@ critical section: the guard, the start time and the writer. Releasing the guard
 reaching for ``writer_mutex_`` afterwards leaves a gap in which the incoming confirmation
 installs its writer and the outgoing finalise then destroys it, after which the new
 recording writes through a null pointer - every message dropped - and still stores a row
-for the empty bag it produced. The writer is only *closed* outside the locks, once it is
-exclusively the finalise's own, because flushing a bag is real I/O.
+for the empty bag it produced. The writer is *closed* only after all three of
+``post_fault_timer_mutex_``, ``capture_topics_mutex_`` and ``writer_mutex_`` are released,
+once it is exclusively the finalise's own, because flushing a bag is real I/O. It is not
+closed unsynchronised, though. It is closed under a different lock, and that lock exists
+for a reason that has nothing to do with this class's state.
+
+Closing a bag, and the lock that is not the data lock
+"""""""""""""""""""""""""""""""""""""""""""""""""""""
+
+``~Writer`` can unload the storage plugin's shared library and ``open()`` loads it, and the
+state both touch is **process-global**: ``class_loader`` keeps one registry of loaded
+libraries keyed by library path, so every writer of a given format shares one
+``rcutils_shared_library_t`` no matter which thread, which ``RosbagCapture`` or which
+``FaultManagerNode`` created it. A close racing an open puts two threads into ``dlclose``
+on the same handle. The winner closes it and zeroes the struct; the loser's ``dlclose``
+reports "shared object not open" and then calls the now-null ``allocator.deallocate``,
+and the process dies in ``rcutils_unload_shared_library`` with the instruction pointer at
+zero. Reproduced outside this package with threads doing nothing but open/close loops on
+one format: four threads x 200 iterations failed in 20 of 20 runs on ``mcap`` and in 20 of
+20 on ``sqlite3``, so it belongs to neither backend and to no single instance.
+
+An instance member cannot serialise process-global state, so the lock is a file-scope
+``plugin_mutex()`` in ``rosbag_capture.cpp``, held across a writer's construction, its
+``open()`` and its destruction, on every path that has one: both finalise paths,
+``discard_active_writer()``, the destructor, and the storage-backend probe that every
+capture runs while it is being constructed. ``open()`` is inside the lock deliberately -
+the constructor reaches no loader, the open is what loads the plugin, and a lock around
+construction and destruction alone still let failures through. The mutex is leaked on
+purpose (a ``new std::mutex`` that is never deleted): a static mutex destroyed during
+static destruction, while another thread is closing a bag, reopens the very window it
+exists to close. With it, the same loops ran 0 failures in 112000 operations on each
+backend.
+
+It is deliberately **not** ``writer_mutex_``. That lock is taken by ``message_callback()``
+for every message of a post-roll and by the flush loop for every buffered message, so
+charging a close to it would stall the capture's own write path for the length of a flush
+plus a ``metadata.yaml`` write. Keeping the close off the data lock was the original
+design's call, and the cost it avoided is real; what it left unpaid was safety. Measured on
+one workstation, single-threaded: a close costs about 0.37 ms for a bag with no messages
+(200 samples) and about 1.1 ms for one holding 256 MB, the ring buffer's default RAM cap,
+split at the default 50 MB per file (12 samples). A recording opening at that moment waits
+behind that. The figure is small because closing flushes to the page cache and writes
+``metadata.yaml``; it does not ``fsync``, so slow or synchronous storage will cost more.
 
 The resulting order is ``node rosbag mutex -> post_fault_timer_mutex_ ->
-{capture_topics_mutex_, writer_mutex_}``, with ``buffer_mutex_`` never held across another
-lock. Paths that take the capture-topics or writer locks on their own release each before
-taking the next, so no reverse edge exists and the order is acyclic.
+{capture_topics_mutex_, writer_mutex_}`` plus ``plugin_mutex() -> writer_mutex_``, with
+``buffer_mutex_`` never held across another lock. ``plugin_mutex()`` is taken in three
+shapes: alone, to destroy a writer already moved out of ``active_writer_``; alone, across a
+probe writer's construction, ``open()`` and destruction in ``default_storage_probe()``,
+which never touches ``active_writer_`` at all; or as the outer of the pair in
+``open_bag_writer()``. No path takes it while holding a lock of this class. That is what
+fixes the shape every destruction site shares - move the writer out of ``active_writer_``
+under ``writer_mutex_``, release ``writer_mutex_``, then destroy under ``plugin_mutex()``.
+Resetting in place under ``writer_mutex_`` would add the reverse edge and deadlock against
+a concurrent ``open_bag_writer()``. That cycle is reachable precisely because the close sits
+outside ``post_fault_timer_mutex_``: a confirmation running at the same time is not held at
+the attach check, so it can be inside ``open_bag_writer()`` holding the plugin lock while
+the finalise holds ``writer_mutex_`` and asks for it. Moving the close back inside
+``post_fault_timer_mutex_`` would remove the cycle and reintroduce the stall the design
+declines to pay for. Paths that take the capture-topics or writer locks on their own release
+each before taking the next, so no other reverse edge exists and the order is acyclic.
 
 Honest durations
 """"""""""""""""