Waybionic · khuzaymahbinharis-jpg · Aug 4, 2026
@@ -3,6 +3,10 @@ build/
 install/
 log/
 
+# Python caches
+__pycache__/
+*.py[cod]
+
 # imported ROS projects
 easy_handeye2/
 ros2_aruco/

@@ -72,6 +72,7 @@ install(
 )
 
 if(BUILD_TESTING)
+  find_package(ament_cmake_gtest REQUIRED)
   find_package(ament_cmake_lint_cmake REQUIRED)
   find_package(ament_cmake_pytest REQUIRED)
   find_package(ament_cmake_xmllint REQUIRED)
@@ -84,6 +85,23 @@ if(BUILD_TESTING)
     test/test_package_metadata.py
     TIMEOUT 60
   )
+
+  # Built from sources directly so the stress test links neither Qt nor RViz and
+  # can run headless.
+  ament_add_gtest(test_ros_diagnostics_source
+    test/test_ros_diagnostics_source.cpp
+    src/ros_diagnostics_source.cpp
+    TIMEOUT 300
+  )
+  if(TARGET test_ros_diagnostics_source)
+    target_include_directories(test_ros_diagnostics_source PRIVATE
+      ${CMAKE_CURRENT_SOURCE_DIR}/include
+    )
+    ament_target_dependencies(test_ros_diagnostics_source
+      diagnostic_msgs
+      rclcpp
+    )
+  endif()
 endif()
 
 ament_export_targets(export_${PROJECT_NAME} HAS_LIBRARY_TARGET)

@@ -55,8 +55,10 @@ waybionic_rviz_plugins/
     temporary_diagnostics_publisher.launch.py
   test/
     test_package_metadata.py
+    test_ros_diagnostics_source.cpp   # Live/mock source handoff stress tests
   docs/
     DIAGNOSTICS_CONTRACT.md
+    DIAGNOSTICS_SOURCE_LIFECYCLE.md
     GROUND_STATION_RVIZ_UI.md
     PR_NOTES.md
 ```
@@ -137,6 +139,8 @@ DiagnosticsSource
 
 `RosDiagnosticsSource` maps ROS diagnostic levels and fields into the internal `DiagnosticMessage` model before the Qt panel renders them. See `docs/DIAGNOSTICS_CONTRACT.md` for the full mapping Korede/backend should follow, and `docs/DIAGNOSTICS_BACKEND_INTEGRATION.md` for backend replacement guidance.
 
+Switching between mock and live replaces the active source while a ROS callback may still be running. `docs/DIAGNOSTICS_SOURCE_LIFECYCLE.md` documents the ownership rules that keep that handoff safe and the stress test that guards it.
+
 ## Platform Notes
 
 - Primary validation target is Ubuntu/WSL2 with ROS 2 Jazzy.
@@ -146,6 +150,7 @@ DiagnosticsSource
 
 - `docs/DIAGNOSTICS_CONTRACT.md` — normalized diagnostic model and ROS mapping
 - `docs/DIAGNOSTICS_BACKEND_INTEGRATION.md` — how a real backend replaces the temporary publisher
+- `docs/DIAGNOSTICS_SOURCE_LIFECYCLE.md` — mock/live source ownership and the handoff stress test
 - `docs/GROUND_STATION_RVIZ_UI.md` — extended architecture notes
 - `docs/PR_NOTES.md` — review summary and PR description source
 

@@ -0,0 +1,128 @@
+# Diagnostics Source Lifecycle
+
+`DiagnosticsPanel` can swap its data source at runtime when the operator toggles
+**Use Mock Diagnostics**, or when launch parameters are applied after RViz
+restores a saved config. This note documents the ownership rules that make that
+swap safe, because the source is read by the Qt thread and written by a ROS
+executor thread at the same time.
+
+## The problem
+
+`RosDiagnosticsSource` owns a subscription to a `diagnostic_msgs/msg/DiagnosticArray`
+topic. Two threads are involved:
+
+| Thread | Work |
+|--------|------|
+| ROS executor | Runs the subscription callback and writes received diagnostics |
+| Qt main thread | Runs the 1 Hz refresh timer, reads diagnostics, and swaps sources |
+
+Switching from live back to mock destroys the live source from the Qt thread. If
+the subscription callback holds a raw pointer to that object, the callback can be
+executing on the executor thread at the moment the object is freed, which is a
+use-after-free.
+
+```mermaid
+sequenceDiagram
+    participant Qt as Qt thread
+    participant Exec as ROS executor
+    participant Src as RosDiagnosticsSource
+    Qt->>Src: configureSource(mock)
+    Exec->>Src: diagnosticsCallback() running
+    Qt->>Src: destroy
+    Exec-->>Src: writes into freed memory
+```
+
+## Ownership rules
+
+1. **Received state is owned separately from the source object.**
+   `RosDiagnosticsSource` keeps its messages in a `SharedState` block held by
+   `shared_ptr`. The subscription callback captures that `shared_ptr` by value
+   instead of capturing `this`, so the callback never dereferences the source
+   object and cannot outlive its own data.
+
+2. **Retirement is explicit.** `DiagnosticsSource::stop()` is a virtual no-op that
+   `RosDiagnosticsSource` overrides. It drops the subscription handle and then
+   sets `active = false` under the state mutex. A callback already mid-write
+   finishes first; any later dispatch of a queued message sees `active == false`
+   and returns without touching the state.
+
+3. **The panel retires before it replaces.** `DiagnosticsPanel::configureSource()`
+   moves the outgoing source into a local variable, calls `stop()` on it, installs
+   the replacement, and only then releases the retired source. This also
+   guarantees a second subscription is never created while the first is still
+   attached.
+
+4. **The refresh timer pins its source.** `DiagnosticsPanel::refresh()` copies the
+   `shared_ptr` for the duration of the tick, so a swap mid-tick cannot leave the
+   widgets reading a half-replaced source.
+
+5. **The panel destructor stops the timer first**, then stops the source, so no
+   callback or timer fires against partly destroyed members.
+
+The UI thread is never blocked for longer than one vector assignment, and no
+source is leaked: `stop()` is idempotent and the destructor calls it.
+
+## Repeatable stress check
+
+`test/test_ros_diagnostics_source.cpp` is the automated equivalent of toggling
+the checkbox repeatedly with the cycle publisher running. It spins a
+`MultiThreadedExecutor` with four threads while a separate thread publishes on
+the topic every 200 microseconds, then creates, reads, stops, and destroys live
+sources in a loop.
+
+| Test | What it proves |
+|------|----------------|
+| `ReportsWaitingBeforeAnyMessageArrives` | Live mode reports a waiting row before traffic |
+| `NormalizesReceivedDiagnostics` | Received statuses map onto the internal contract |
+| `StopFreezesStateAndIgnoresLaterMessages` | A retired source stops ingesting messages |
+| `StopIsIdempotent` | Repeated `stop()` calls and later reads are safe |
+| `RepeatedLiveMockChurnUnderTrafficIsSafe` | 300 create/stop/destroy cycles under load |
+| `DestructionWithoutExplicitStopIsSafe` | The destructor alone retires the subscription |
+| `ConcurrentReadsDuringTeardownAreSafe` | A reader thread racing `stop()` is safe |
+| `ChurnLeavesNoLingeringSubscription` | No subscription leak after repeated churn |
+
+Run it with:
+
+```bash
+source /opt/ros/jazzy/setup.bash
+colcon build --packages-select waybionic_rviz_plugins --symlink-install
+colcon test --packages-select waybionic_rviz_plugins
+colcon test-result --all --verbose
+```
+
+### Confirming the stress check actually detects the race
+
+A stress test that passes against both the broken and the fixed code proves
+nothing, so the suite was validated by mutation under AddressSanitizer. The
+stress test and `src/ros_diagnostics_source.cpp` were compiled standalone twice:
+once against the current source, and once against a mutated copy whose
+subscription callback captures `this` and whose `stop()` does not drop the
+subscription, which is the ownership model that existed before this change.
+
+| Build | Result |
+|-------|--------|
+| Current source | 4 stress tests pass |
+| Mutated to capture `this` | `AddressSanitizer: heap-use-after-free` in `std::__shared_ptr<SharedState>::get()`, raised by `ConcurrentReadsDuringTeardownAreSafe` |
+
+`new_delete_type_mismatch` has to be disabled for these runs because rclcpp's
+internal C allocator shim trips it during plain node construction, unrelated to
+this code:
+
+```bash
+ASAN_OPTIONS=detect_leaks=0:new_delete_type_mismatch=0 ./test_ros_diagnostics_source
+```
+
+### Manual GUI equivalent
+
+```bash
+# Terminal 1
+ros2 launch waybionic_rviz_plugins temporary_diagnostics_publisher.launch.py mode:=cycle
+
+# Terminal 2
+ros2 launch waybionic_rviz_plugins engineer_view.launch.py use_mock_diagnostics:=false
+```
+
+Toggle **Use Mock Diagnostics** on and off repeatedly while messages arrive. Expected
+behaviour: no crash or freeze, the source label alternates between `Mock` and
+`ROS /diagnostics`, the mock buttons enable only in mock mode, and live rows resume
+updating each time live mode is re-selected.
@@ -33,6 +33,7 @@ class DiagnosticsPanel : public rviz_common::Panel
 
 public:
   explicit DiagnosticsPanel(QWidget * parent = nullptr);
+  ~DiagnosticsPanel() override;
 
   void onInitialize() override;
   void save(rviz_common::Config config) const override;
@@ -46,7 +47,10 @@ class DiagnosticsPanel : public rviz_common::Panel
   void refresh();
   void setMockDiagnosticsState(MockDiagnosticsState mode);
   void setUseMockDiagnostics(bool use_mock_diagnostics);
-  void updateSystemStatus(const std::vector<DiagnosticMessage> & messages, const rclcpp::Time & now);
+  void updateSystemStatus(
+    const DiagnosticsSource & source,
+    const std::vector<DiagnosticMessage> & messages,
+    const rclcpp::Time & now);
   void updateTelemetryTable(const std::vector<DiagnosticMessage> & messages, const rclcpp::Time & now);
   void updateAlerts(const std::vector<DiagnosticMessage> & messages);
   void updateSourceControls();
@@ -58,8 +62,10 @@ class DiagnosticsPanel : public rviz_common::Panel
   QString optionalText(const std::optional<std::string> & value) const;
   QString alertText(const DiagnosticMessage & message) const;
 
-  std::unique_ptr<DiagnosticsSource> diagnostics_source_;
-  MockDiagnosticsSource * mock_diagnostics_source_{nullptr};
+  // Shared ownership so a refresh tick keeps its source alive even if a mode
+  // switch replaces the panel's source part-way through the tick.
+  std::shared_ptr<DiagnosticsSource> diagnostics_source_;
+  std::shared_ptr<MockDiagnosticsSource> mock_diagnostics_source_;
   rclcpp::Node::SharedPtr rviz_node_;
   rclcpp::Clock clock_{RCL_SYSTEM_TIME};
   std::string diagnostics_topic_{"/diagnostics"};

@@ -16,6 +16,10 @@ class DiagnosticsSource
 public:
   virtual ~DiagnosticsSource() = default;
 
+  /// Detach any external data feed so no further callbacks mutate this source.
+  /// Must be idempotent and safe to call from the UI thread.
+  virtual void stop() {}
+
   virtual std::string sourceName() const = 0;
   virtual std::string connectionStatus(const rclcpp::Time & now) const = 0;
   virtual std::vector<DiagnosticMessage> messages(const rclcpp::Time & now) const = 0;

@@ -1,6 +1,7 @@
 #ifndef WAYBIONIC_RVIZ_PLUGINS__ROS_DIAGNOSTICS_SOURCE_HPP_
 #define WAYBIONIC_RVIZ_PLUGINS__ROS_DIAGNOSTICS_SOURCE_HPP_
 
+#include <memory>
 #include <mutex>
 #include <string>
 #include <vector>
@@ -13,31 +14,57 @@
 namespace waybionic_rviz_plugins
 {
 
+/// Live diagnostics source backed by a ROS 2 subscription.
+///
+/// The subscription callback runs on an executor thread while the RViz panel
+/// reads and replaces sources from the Qt thread. Received data therefore lives
+/// in a separately owned state block that the callback keeps alive, so tearing
+/// this object down can never leave an in-flight callback writing into freed
+/// memory.
 class RosDiagnosticsSource : public DiagnosticsSource
 {
 public:
   explicit RosDiagnosticsSource(
     rclcpp::Node::SharedPtr node,
     std::string diagnostics_topic = "/diagnostics");
 
+  ~RosDiagnosticsSource() override;
+
+  RosDiagnosticsSource(const RosDiagnosticsSource &) = delete;
+  RosDiagnosticsSource & operator=(const RosDiagnosticsSource &) = delete;
+  RosDiagnosticsSource(RosDiagnosticsSource &&) = delete;
+  RosDiagnosticsSource & operator=(RosDiagnosticsSource &&) = delete;
+
+  /// Drops the subscription and retires the shared state. Any callback that is
+  /// already executing finishes against the retired state and is discarded.
+  void stop() override;
+
+  bool isStopped() const;
+
   std::string sourceName() const override;
   std::string connectionStatus(const rclcpp::Time & now) const override;
   std::vector<DiagnosticMessage> messages(const rclcpp::Time & now) const override;
 
 private:
-  void diagnosticsCallback(diagnostic_msgs::msg::DiagnosticArray::SharedPtr message);
-  DiagnosticMessage toDiagnosticMessage(
-    const diagnostic_msgs::msg::DiagnosticStatus & status,
-    const rclcpp::Time & timestamp) const;
+  /// Data shared between the executor thread and the Qt thread. Held by
+  /// shared_ptr so the subscription callback always operates on live memory.
+  struct SharedState
+  {
+    mutable std::mutex mutex;
+    std::vector<DiagnosticMessage> latest_messages;
+    rclcpp::Time last_received_time{0, 0, RCL_SYSTEM_TIME};
+    bool has_received{false};
+    bool active{true};
+  };
+
+  static void handleMessage(
+    const std::shared_ptr<SharedState> & state,
+    const diagnostic_msgs::msg::DiagnosticArray & message);
 
   rclcpp::Node::SharedPtr node_;
   std::string diagnostics_topic_;
   rclcpp::Subscription<diagnostic_msgs::msg::DiagnosticArray>::SharedPtr subscription_;
-
-  mutable std::mutex mutex_;
-  std::vector<DiagnosticMessage> latest_messages_;
-  rclcpp::Time last_received_time_{0, 0, RCL_SYSTEM_TIME};
-  bool has_received_{false};
+  std::shared_ptr<SharedState> state_;
 };
 
 }  // namespace waybionic_rviz_plugins

@@ -20,6 +20,7 @@
   <exec_depend>rclpy</exec_depend>
   <exec_depend>rviz2</exec_depend>
 
+  <test_depend>ament_cmake_gtest</test_depend>
   <test_depend>ament_cmake_lint_cmake</test_depend>
   <test_depend>ament_cmake_pytest</test_depend>
   <test_depend>ament_cmake_xmllint</test_depend>