refactor(FileLogger2): proper thread sync with minimal TSAN suppression

Changes:
- Replace spin-wait with condition_variable for thread startup sync
- Keep ready check in callback to prevent deadlock during construction
- Delete move ops in base class for consistency with derived class
- Add minimal TSAN suppression for writerLoop only (TSAN limitation)

The callback must check the ready flag because the constructor holds
queue_mutex while waiting for the writer thread. Without this check,
callbacks during tree.tickWhileRunning() would deadlock trying to
acquire the same mutex.

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

Author: facontidavide

include/behaviortree_cpp/loggers/bt_file_logger_v2.h

@@ -24,8 +24,8 @@ struct FileLogger2PImplBase
   ~FileLogger2PImplBase();
   FileLogger2PImplBase(const FileLogger2PImplBase&) = delete;
   FileLogger2PImplBase& operator=(const FileLogger2PImplBase&) = delete;
-  FileLogger2PImplBase(FileLogger2PImplBase&&) = default;
-  FileLogger2PImplBase& operator=(FileLogger2PImplBase&&) = default;
+  FileLogger2PImplBase(FileLogger2PImplBase&&) = delete;
+  FileLogger2PImplBase& operator=(FileLogger2PImplBase&&) = delete;
 };
 }  // namespace detail
 

src/loggers/bt_file_logger_v2.cpp

@@ -38,10 +38,8 @@ struct detail::FileLogger2Private
 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);
+  // _p is now ready for use. StatusChangeLogger (constructed next) will
+  // subscribe to callbacks that may immediately start queuing transitions.
 }
 
 // Destructor must be defined where FileLogger2Private is complete
@@ -95,10 +93,11 @@ FileLogger2::FileLogger2(const BT::Tree& tree, std::filesystem::path const& file
 
   _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))
+  // Wait for writer thread to signal it's ready using condition variable
   {
-    std::this_thread::yield();
+    std::unique_lock lock(_p->queue_mutex);
+    _p->queue_cv.wait(lock,
+                      [this]() { return _p->ready.load(std::memory_order_relaxed); });
   }
 }
 
@@ -113,8 +112,8 @@ 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.
+  // Don't queue callbacks until writer thread is ready - the constructor
+  // holds queue_mutex while waiting, so we'd deadlock.
   if(!_p->ready.load(std::memory_order_acquire))
   {
     return;
@@ -142,13 +141,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.
+  // Signal that the writer thread is ready.
+  // The mutex + notify establishes happens-before with the constructor's wait.
   {
     std::scoped_lock lock(_p->queue_mutex);
-    _p->ready.store(true, std::memory_order_release);
+    _p->ready.store(true, std::memory_order_relaxed);
   }
+  _p->queue_cv.notify_all();
 
   while(_p->loop)
   {

tests/tsan_suppressions.txt

@@ -3,12 +3,10 @@
 # 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
+# FileLogger2: TSAN doesn't track happens-before between make_unique and
+# first mutex access in spawned thread. The synchronization is correct:
+# - Base class creates _p (with mutex) before StatusChangeLogger subscribes
+# - Writer thread signals ready only after acquiring mutex
+# - Constructor waits on ready before returning
+# This is a TSAN limitation, not a real race.
 race:BT::FileLogger2::writerLoop