refactor(FileLogger2): proper thread sync without TSAN suppressions

Changes:
- Replace spin-wait with condition_variable for thread startup sync
- Remove early callback dropping - queue is always safe to access
- Remove TSAN suppressions - proper sync approach doesn't need them
- Delete move ops in base class for consistency with derived class
- Simplify base constructor - no mutex hack needed

The key insight: the queue is protected by queue_mutex and initialized
before StatusChangeLogger subscribes to callbacks (base-from-member
idiom), so callbacks can safely enqueue at any time.

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,13 +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.
-  if(!_p->ready.load(std::memory_order_acquire))
-  {
-    return;
-  }
-
+  // The queue is always safe to access - it's protected by queue_mutex and
+  // initialized before StatusChangeLogger subscribes to callbacks.
   Transition trans{};
   trans.timestamp_usec = uint64_t(ToUsec(timestamp - _p->first_timestamp));
   trans.node_uid = node.UID();
@@ -142,13 +136,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

@@ -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