Bugfix: C++ Recorder fails on stop()->record() due to reusing bag name

Signed-off-by: Michael Orlov <[email protected]>

Author: MichaelOrlov

rosbag2_transport/src/rosbag2_transport/recorder.cpp

@@ -383,6 +383,31 @@ void RecorderImpl::record()
   subscriptions_.clear();
   event_notifier_->reset_total_num_messages_lost_in_transport();
   event_notifier_->reset_total_num_messages_lost_in_recorder();
+
+  // Check if storage_options.uri already exists and try to add '(n)' postfix
+  namespace fs = std::filesystem;
+  fs::path storage_path(storage_options_.uri);
+  if (fs::is_directory(storage_path)) {
+    RCLCPP_WARN_STREAM(node->get_logger(),
+                       "Bag directory '" << storage_path.c_str() << "' already exists.");
+    for (size_t i = 1U; i < std::numeric_limits<size_t>::max(); i++) {
+      fs::path new_path = storage_path;
+      new_path += "(" + std::to_string(i) + ")";
+      if (!fs::exists(new_path)) {
+        RCLCPP_WARN_STREAM(node->get_logger(),
+                           "Changing bag directory to '" << new_path.c_str() << "'.");
+        storage_options_.uri = new_path.generic_string();
+        storage_path = new_path;
+        break;
+      }
+    }
+  }
+  if (fs::is_directory(storage_path)) {
+    throw std::runtime_error{
+            "Failed to derive non-existent directory for the new Rosbag2 recording. "
+            "Please specify non existent uri explicitly."};
+  }
+
   writer_->open(
     storage_options_,
     {record_options_.input_serialization_format, record_options_.output_serialization_format});

rosbag2_transport/test/rosbag2_transport/test_record.cpp

@@ -14,17 +14,21 @@
 
 #include <gmock/gmock.h>
 
+#include <filesystem>
 #include <memory>
 #include <string>
 #include <unordered_map>
 #include <utility>
 #include <vector>
 #include <rosbag2_storage/ros_helper.hpp>
 
+#include "mock_recorder.hpp"
 #include "rclcpp/rclcpp.hpp"
+#include "rcpputils/scope_exit.hpp"
 
 #include "rosbag2_test_common/publication_manager.hpp"
 #include "rosbag2_test_common/wait_for.hpp"
+#include "rosbag2_test_common/temporary_directory_fixture.hpp"
 
 #include "rosbag2_transport/recorder.hpp"
 
@@ -34,6 +38,11 @@
 
 #include "rosbag2_storage/qos.hpp"
 #include "record_integration_fixture.hpp"
+#include "rosbag2_transport/reader_writer_factory.hpp"
+
+using namespace ::testing;  // NOLINT
+using rosbag2_test_common::TemporaryDirectoryFixture;
+namespace fs = std::filesystem;
 
 TEST_F(RecordIntegrationTestFixture, published_messages_from_multiple_topics_are_recorded)
 {
@@ -126,6 +135,33 @@ TEST_F(RecordIntegrationTestFixture, published_messages_from_multiple_topics_are
   }
 }
 
+TEST_F(TemporaryDirectoryFixture, can_record_again_after_stop_with_real_storage) {
+  rclcpp::init(0, nullptr);
+  auto cleanup_process_handle = rcpputils::make_scope_exit([&]() {rclcpp::shutdown();});
+  std::string test_topic = "/can_record_again_after_stop_topic";
+  rosbag2_storage::StorageOptions storage_options{};
+  storage_options.uri = (fs::path(temporary_dir_path_) / "start_stop_again").generic_string();
+
+  rosbag2_transport::RecordOptions record_options{};
+
+  auto writer = rosbag2_transport::ReaderWriterFactory::make_writer(record_options);
+  {
+    auto recorder = std::make_shared<MockRecorder>(
+      std::move(writer), storage_options, record_options);
+
+    EXPECT_NO_THROW(recorder->record());
+    fs::path storage_path(storage_options.uri);
+    EXPECT_TRUE(fs::is_directory(storage_path));
+
+    EXPECT_NO_THROW(recorder->stop());
+    EXPECT_NO_THROW(recorder->record());
+    storage_path = recorder->get_storage_options().uri;
+    EXPECT_TRUE(fs::is_directory(storage_path));
+    std::string expected_path_str = storage_options.uri + "(1)";
+    EXPECT_EQ(storage_path.generic_string(), expected_path_str);
+  }
+}
+
 TEST_F(RecordIntegrationTestFixture, can_record_again_after_stop)
 {
   GTEST_SKIP() << "Skipping test `can_record_again_after_stop` in rosbag2_transport, until "