Bag file size may be significantly larger than `--max-bag-size` when recording with cache

[MichaelOrlov]

Description

Bag file size may be significantly larger than --max-bag-size when recording with cache

Expected Behavior

The bag file size shall not be "significantly" larger (more than one message size) than the value specified in the --max-bag-size when recording with the enabled cache, i.e., with --max-cache-size > 0

Actual Behavior

Sometimes, the bag file size can be significantly larger than the set limit in the --max-bag-size CLI option.

To Reproduce

** Steps to reproduce the behavior, e.g.
Run existing RecordFixture::record_end_to_end_with_splitting_bagsize_split_is_at_least_specified_size test locally:

rosbag2/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp

Lines 332 to 379 in 4da63db

	TEST_P(RecordFixture, record_end_to_end_with_splitting_max_size_not_reached) {
	constexpr const char topic_name[] = "/test_topic";
	constexpr const int bagfile_split_size = 4 * 1024 * 1024; // 4MB.
	constexpr const int message_size = 512 * 1024; // 512KB
	constexpr const char message_str[] = "Test";
	const auto message = create_string_message(message_str, message_size);
	// only fill the bagfile halfway
	constexpr const int message_count = bagfile_split_size / message_size / 2;
	rosbag2_test_common::PublicationManager pub_manager;
	pub_manager.setup_publisher(topic_name, message, message_count);
	std::stringstream command;
	command << get_base_record_command() <<
	" --max-bag-size " << bagfile_split_size <<
	" --topics " << topic_name;
	auto process_handle = start_execution(command.str());
	auto cleanup_process_handle = rcpputils::make_scope_exit(
	[process_handle]() {
	stop_execution(process_handle);
	});
	ASSERT_TRUE(pub_manager.wait_for_matched(topic_name)) <<
	"Expected find rosbag subscription";
	wait_for_storage_file();
	pub_manager.run_publishers();
	stop_execution(process_handle);
	cleanup_process_handle.cancel();
	finalize_metadata_kludge();
	wait_for_metadata();
	rosbag2_storage::MetadataIo metadata_io;
	const auto metadata = metadata_io.read_metadata(root_bag_path_.generic_string());
	// Check that there's only 1 bagfile and that it exists.
	ASSERT_EQ(1u, metadata.files.size());
	const auto bagfile_path = root_bag_path_ / fs::path{metadata.files[0].path};
	ASSERT_TRUE(fs::exists(bagfile_path)) <<
	"Expected bag file: \"" << bagfile_path.generic_string() << "\" to exist.";
	// Check that the next bagfile does not exist.
	const auto next_bag_file = get_bag_file_path(1);
	EXPECT_FALSE(fs::exists(next_bag_file)) << "Expected next bag file: \"" <<
	next_bag_file.generic_string() << "\" to not exist!";
	}

System (please complete the following information)

• OS: Ubuntu 22.04
• ROS 2 Distro: Rolling, Kilted, Jazzy
• Install Method: any
• Version: 4da63db

Additional context

For some unknown reason, the test record_end_to_end_with_splitting_bagsize_split_is_at_least_specified_size is not failing on CI.

RCA

1. The root cause is that we are not taking into account messages currently residing in the cache when doing an estimate for the file size when checking if we need to make a split
   

   rosbag2/rosbag2_cpp/src/rosbag2_cpp/writers/sequential_writer.cpp

   Lines 468 to 479 in 4da63db

   	bool SequentialWriter::should_split_bagfile(
   	const std::chrono::time_point<std::chrono::high_resolution_clock> & current_time) const
   	{
   	// Assume we aren't splitting
   	bool should_split = false;
   	// Splitting by size
   	if (storage_options_.max_bagfile_size !=
   	rosbag2_storage::storage_interfaces::MAX_BAGFILE_SIZE_NO_SPLIT)
   	{
   	should_split = (storage_->get_bagfile_size() >= storage_options_.max_bagfile_size);
   	}

2. The test also fails because we are using a default rclcpp::QoS{rclcpp::KeepAll() for the publishers, and not all messages may be delivered. To make the test more robust need to specify QoS settings for the publisher manager explicitly as rclcpp::QoS qos = rclcpp::SystemDefaultsQoS().keep_last(message_count).reliable();

[MichaelOrlov]

As an intermittent workaround to make test non-flaky need to:

1. Use QoS settings for the publisher manager explicitly as rclcpp::QoS qos = rclcpp::SystemDefaultsQoS().keep_last(message_count).reliable();
2. Add " --max-cache-size 0` to the CLI "command" string. To run a test with the disabled writer's cache.