refactor(StatusChangeLogger): add deferred subscription for thread safety

Add support for deferred subscription in StatusChangeLogger to fix race
conditions in loggers with worker threads (FileLogger2, SqliteLogger).

Changes to StatusChangeLogger:
- Add protected default constructor for deferred subscription mode
- Add protected subscribeToTreeChanges() method
- Derived classes can now complete initialization before receiving callbacks

Changes to FileLogger2:
- Remove FileLogger2PImplBase base-from-member idiom hack
- Use simple Pimpl pattern with deferred subscription
- Subscribe to tree changes AFTER writer thread is started
- Remove ready flag and related synchronization complexity

Changes to SqliteLogger:
- Use deferred subscription to fix same race condition

Remove all FileLogger2 TSAN suppressions - no longer needed.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Author: facontidavide

include/behaviortree_cpp/loggers/abstract_logger.h

@@ -15,14 +15,19 @@ enum class TimestampType
 class StatusChangeLogger
 {
 public:
+  /**
+   * @brief Construct and immediately subscribe to status changes.
+   * Use this for simple loggers that don't need deferred subscription.
+   */
   StatusChangeLogger(TreeNode* root_node);
+
   virtual ~StatusChangeLogger() = default;
 
   StatusChangeLogger(const StatusChangeLogger& other) = delete;
   StatusChangeLogger& operator=(const StatusChangeLogger& other) = delete;
 
-  StatusChangeLogger(StatusChangeLogger&& other) = default;
-  StatusChangeLogger& operator=(StatusChangeLogger&& other) = default;
+  StatusChangeLogger(StatusChangeLogger&& other) = delete;
+  StatusChangeLogger& operator=(StatusChangeLogger&& other) = delete;
 
   virtual void callback(BT::Duration timestamp, const TreeNode& node,
                         NodeStatus prev_status, NodeStatus status) = 0;
@@ -55,6 +60,25 @@ class StatusChangeLogger
     show_transition_to_idle_ = enable;
   }
 
+protected:
+  /**
+   * @brief Default constructor for deferred subscription mode.
+   *
+   * Use this when the derived class needs to complete initialization
+   * (e.g., start worker threads) before receiving callbacks.
+   * Call subscribeToTreeChanges() when ready to receive callbacks.
+   */
+  StatusChangeLogger() = default;
+
+  /**
+   * @brief Subscribe to status changes on the tree.
+   *
+   * For loggers with threading or complex initialization, call this
+   * at the END of your constructor after all setup is complete.
+   * This ensures callbacks won't fire until you're ready to handle them.
+   */
+  void subscribeToTreeChanges(TreeNode* root_node);
+
 private:
   bool enabled_ = true;
   bool show_transition_to_idle_ = true;
@@ -67,6 +91,11 @@ class StatusChangeLogger
 //--------------------------------------------
 
 inline StatusChangeLogger::StatusChangeLogger(TreeNode* root_node)
+{
+  subscribeToTreeChanges(root_node);
+}
+
+inline void StatusChangeLogger::subscribeToTreeChanges(TreeNode* root_node)
 {
   first_timestamp_ = std::chrono::high_resolution_clock::now();
 

include/behaviortree_cpp/loggers/bt_file_logger_v2.h

@@ -1,34 +1,12 @@
 #pragma once
 #include "behaviortree_cpp/loggers/abstract_logger.h"
 
-#include <array>
-#include <deque>
 #include <filesystem>
-#include <fstream>
+#include <memory>
 
 namespace BT
 {
 
-// Forward declaration for base-from-member idiom
-namespace detail
-{
-struct FileLogger2Private;
-
-// This base class ensures _p is initialized BEFORE StatusChangeLogger,
-// which subscribes to callbacks in its constructor. Without this,
-// callbacks could fire before _p exists (C++ initializes bases before members).
-struct FileLogger2PImplBase
-{
-  std::unique_ptr<FileLogger2Private> _p;
-  FileLogger2PImplBase();
-  ~FileLogger2PImplBase();
-  FileLogger2PImplBase(const FileLogger2PImplBase&) = delete;
-  FileLogger2PImplBase& operator=(const FileLogger2PImplBase&) = delete;
-  FileLogger2PImplBase(FileLogger2PImplBase&&) = default;
-  FileLogger2PImplBase& operator=(FileLogger2PImplBase&&) = default;
-};
-}  // namespace detail
-
 /**
  * @brief The FileLogger2 is a logger that saves the tree as
  * XML and all the transitions.
@@ -42,7 +20,7 @@ struct FileLogger2PImplBase
  * - next: each 9 bytes is a FileLogger2::Transition. See definition.
  *
  */
-class FileLogger2 : private detail::FileLogger2PImplBase, public StatusChangeLogger
+class FileLogger2 : public StatusChangeLogger
 {
 public:
   /**
@@ -79,6 +57,9 @@ class FileLogger2 : private detail::FileLogger2PImplBase, public StatusChangeLog
   void flush() override;
 
 private:
+  struct Pimpl;
+  std::unique_ptr<Pimpl> _p;
+
   void writerLoop();
 };
 

src/loggers/bt_file_logger_v2.cpp

@@ -2,6 +2,14 @@
 
 #include "behaviortree_cpp/xml_parsing.h"
 
+#include <array>
+#include <condition_variable>
+#include <cstring>
+#include <deque>
+#include <fstream>
+#include <mutex>
+#include <thread>
+
 #include "flatbuffers/base.h"
 
 namespace BT
@@ -16,7 +24,7 @@ int64_t ToUsec(Duration ts)
 }  // namespace
 
 // Define the private implementation struct
-struct detail::FileLogger2Private
+struct FileLogger2::Pimpl
 {
   std::ofstream file_stream;
   std::mutex file_mutex;  // Protects file_stream access from multiple threads
@@ -29,32 +37,12 @@ struct detail::FileLogger2Private
 
   std::thread writer_thread;
   std::atomic_bool loop = true;
-
-  // Atomic flag for thread startup - callbacks check this before accessing queue
-  std::atomic_bool ready = false;
 };
 
-// Base class constructor - initializes _p before StatusChangeLogger
-detail::FileLogger2PImplBase::FileLogger2PImplBase()
-  : _p(std::make_unique<FileLogger2Private>())
-{
-  // Acquire mutex once to establish happens-before relationship for TSAN.
-  // This must happen before StatusChangeLogger's constructor subscribes to
-  // callbacks that may access the mutex from other threads.
-  std::scoped_lock lock(_p->queue_mutex);
-}
-
-// Destructor must be defined where FileLogger2Private is complete
-detail::FileLogger2PImplBase::~FileLogger2PImplBase() = default;
-
 FileLogger2::FileLogger2(const BT::Tree& tree, std::filesystem::path const& filepath)
-  : StatusChangeLogger(tree.rootNode())
+  : StatusChangeLogger()  // Deferred subscription - don't subscribe yet
+  , _p(std::make_unique<Pimpl>())
 {
-  // Note: _p is already initialized by FileLogger2PImplBase before
-  // StatusChangeLogger's constructor runs. This is critical because
-  // StatusChangeLogger subscribes to callbacks in its constructor,
-  // and those callbacks may access _p.
-
   if(filepath.filename().extension() != ".btlog")
   {
     throw RuntimeError("FileLogger2: the file extension must be [.btlog]");
@@ -93,13 +81,12 @@ FileLogger2::FileLogger2(const BT::Tree& tree, std::filesystem::path const& file
   flatbuffers::WriteScalar(write_buffer.data(), timestamp_usec);
   _p->file_stream.write(write_buffer.data(), 8);
 
+  // Start the writer thread
   _p->writer_thread = std::thread(&FileLogger2::writerLoop, this);
 
-  // Wait for writer thread to signal it's ready
-  while(!_p->ready.load(std::memory_order_acquire))
-  {
-    std::this_thread::yield();
-  }
+  // NOW subscribe to tree changes - after everything is initialized and
+  // the writer thread is running. This ensures callbacks are safe to handle.
+  subscribeToTreeChanges(tree.rootNode());
 }
 
 FileLogger2::~FileLogger2()
@@ -113,13 +100,6 @@ FileLogger2::~FileLogger2()
 void FileLogger2::callback(Duration timestamp, const TreeNode& node,
                            NodeStatus /*prev_status*/, NodeStatus status)
 {
-  // Ignore callbacks that arrive before the writer thread is ready.
-  // This can happen during StatusChangeLogger construction.
-  if(!_p->ready.load(std::memory_order_acquire))
-  {
-    return;
-  }
-
   Transition trans{};
   trans.timestamp_usec = uint64_t(ToUsec(timestamp - _p->first_timestamp));
   trans.node_uid = node.UID();
@@ -142,21 +122,13 @@ void FileLogger2::writerLoop()
   // local buffer in this thread
   std::deque<Transition> transitions;
 
-  // Signal that the writer thread is ready to accept callbacks.
-  // IMPORTANT: This must happen AFTER acquiring the mutex for the first time,
-  // to establish proper synchronization with the callback thread.
-  {
-    std::scoped_lock lock(_p->queue_mutex);
-    _p->ready.store(true, std::memory_order_release);
-  }
-
   while(_p->loop)
   {
     transitions.clear();
     {
       std::unique_lock lock(_p->queue_mutex);
       _p->queue_cv.wait_for(lock, std::chrono::milliseconds(10), [this]() {
-        return !_p->transitions_queue.empty() && _p->loop;
+        return !_p->transitions_queue.empty() || !_p->loop;
       });
       // simple way to pop all the transitions from _p->transitions_queue into transitions
       std::swap(transitions, _p->transitions_queue);

src/loggers/bt_sqlite_logger.cpp

@@ -57,7 +57,7 @@ void execStatement(sqlite3_stmt* stmt)
 
 SqliteLogger::SqliteLogger(const Tree& tree, std::filesystem::path const& filepath,
                            bool append)
-  : StatusChangeLogger(tree.rootNode())
+  : StatusChangeLogger()  // Deferred subscription - don't subscribe yet
 {
   const auto extension = filepath.filename().extension();
   if(extension != ".db3" && extension != ".btdb")
@@ -129,6 +129,10 @@ SqliteLogger::SqliteLogger(const Tree& tree, std::filesystem::path const& filepa
   }
 
   writer_thread_ = std::thread(&SqliteLogger::writerLoop, this);
+
+  // NOW subscribe to tree changes - after everything is initialized and
+  // the writer thread is running. This ensures callbacks are safe to handle.
+  subscribeToTreeChanges(tree.rootNode());
 }
 
 SqliteLogger::~SqliteLogger()

tests/tsan_suppressions.txt

@@ -2,13 +2,3 @@
 
 # ZeroMQ false positives
 race:zmq::epoll_t::add_fd
-
-# FileLogger2 false positive: TSAN doesn't properly track happens-before
-# relationship between mutex creation in make_unique and first cross-thread
-# access, even though std::thread::thread provides the synchronization.
-# The mutex is properly protected: base class constructor acquires/releases it
-# before StatusChangeLogger subscribes to callbacks, and writer thread acquires
-# it before setting ready flag.
-mutex:BT::FileLogger2
-race:BT::FileLogger2::callback
-race:BT::FileLogger2::writerLoop