Windows CI can't find metadata.yaml file when using ros2 bag record

[adityapande-1995]

Description

This came up in ros2/examples#327, in the script rosbag_recrd_launch_test.py. The test runs ros2 bag record -a -o <temp_directory> with a talker node, waits for 3 secs for the data to be recorded then checks for metadata.yaml file in the <temp_directory>

Expected Behavior

The Windows CI should find the metadata.yaml file. Linux and MacOS CI work just fine.

To reproduce the error :

Run the following script:

import os
import shutil
import tempfile
import time
import unittest

import launch
import launch.actions
import launch_ros.actions
import launch_testing.actions
import launch_testing.markers
import pytest

import yaml


@pytest.mark.launch_test
@launch_testing.markers.keep_alive
def generate_test_description():
    rosbag_dir = os.path.join(tempfile.mkdtemp(), 'test_bag')

    node_list = [
        launch_ros.actions.Node(
            executable='talker',
            package='demo_nodes_cpp',
            name='demo_node'
        ),
        launch.actions.ExecuteProcess(
            cmd=['ros2', 'bag', 'record', '-a', '-o', rosbag_dir],
            output='screen'
        ),
        launch_testing.actions.ReadyToTest()
    ]

    return launch.LaunchDescription(node_list), {'rosbag_dir': rosbag_dir}


class DelayShutdown(unittest.TestCase):

    def test_delay(self):
        """Delay the shutdown of processes so that rosbag can record some messages."""
        time.sleep(3)


@launch_testing.post_shutdown_test()
class TestFixtureAfterShutdown(unittest.TestCase):

    rosbag_dir = None

    def test_rosbag_record(self, rosbag_dir):
        """Check if the rosbag2 recording was successful."""
        with open(os.path.join(rosbag_dir, 'metadata.yaml'), 'r') as file:
            metadata = yaml.safe_load(file)
            assert metadata['rosbag2_bagfile_information']['message_count'] > 0
            print('The following topics received messages:')
            for item in metadata['rosbag2_bagfile_information']['topics_with_message_count']:
                print(item['topic_metadata']['name'], 'recieved ', item['message_count'],
                      ' messages')

        TestFixtureAfterShutdown.rosbag_dir = rosbag_dir

    @classmethod
    def tearDownClass(cls):
        """Delete the rosbag directory."""
        print('Deleting ', cls.rosbag_dir)
        shutil.rmtree(cls.rosbag_dir.replace('test_bag', ''))

Actual Behavior

• Failing build : https://ci.ros2.org/job/ci_windows/15711/testReport/junit/launch_testing_examples.launch_testing_examples/record_rosbag_launch_test/launch_testing_examples_record_rosbag_launch_test/
• CI error log:

Error Message
launch_testing_examples.record_rosbag_launch_test.TestFixtureAfterShutdown.test_rosbag_record failed
Stacktrace
===============================================================================================================================================================
FAIL: launch_testing_examples.record_rosbag_launch_test.TestFixtureAfterShutdown.test_rosbag_record
---------------------------------------------------------------------------------------------------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\ci\ws\src\ros2\examples\launch_testing\launch_testing_examples\launch_testing_examples\record_rosbag_launch_test.py", line 66, in test_rosbag_record
    with open(rosbag_dir + '/metadata.yaml', 'r') as file:
FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\tmp60w8hd1r/test_bag/metadata.yaml'

System (please complete the following information)

• OS: CI windows
• ROS 2 Distro: Rolling

Additional context

The example script has been disabled for windows for now.

[emersonknapp]

Can't remember exactly - but I think this is a problem we've run into on Windows in the past - that windows can't send a proper SIGINT. So, the ExecuteProcess does not receive the SIGINT and does not have a clean shutdown, which is how the metadata.yaml file is created at the end of the ros2 bag record command.

Not 100% positive that's what is happening, but it is ringing that bell. I don't know how it is solved because Windows doesn't have the same concept of signals. How could we provide a clean shutdown for the process on Windows?

[ivanpauno]

Can't remember exactly - but I think this is a problem we've run into on Windows in the past

Maybe that was this issue

rosbag2/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp

Lines 151 to 164 in a060c1d

	// TODO(Martin-Idel-SI): Find out how to correctly send a Ctrl-C signal on Windows
	// This is necessary as the process is killed hard on Windows and doesn't write a metadata file
	#ifdef _WIN32
	rosbag2_storage::BagMetadata metadata{};
	metadata.version = 1;
	metadata.storage_identifier = "sqlite3";
	metadata.relative_file_paths = {get_bag_file_path(0).string()};
	metadata.duration = std::chrono::nanoseconds(0);
	metadata.starting_time =
	std::chrono::time_point<std::chrono::high_resolution_clock>(std::chrono::nanoseconds(0));
	metadata.message_count = 0;
	rosbag2_storage::MetadataIo metadata_io;
	metadata_io.write_metadata(root_bag_path_.string(), metadata);
	#endif

(?).

In that case, maybe a similar patch can be added in the place where the issue is happening now.

[adityapande-1995]

@ivanpauno for launch_testing, maybe @launch_testing.post_shutdown_test() is a good place to add this change ?

[ivanpauno]

@ivanpauno for launch_testing, maybe @launch_testing.post_shutdown_test() is a good place to add this change ?

maybe, I don't have much context

[hidmic]

maybe @launch_testing.post_shutdown_test() is a good place to add this change ?

That should do.

How could we provide a clean shutdown for the process on Windows?

To the best of my knowledge, there's no easy, non-intrusive mechanism to ask a process to shutdown in Windows. The underlying process (e.g. ros2 bag) would have to expose some sort of discoverable shutdown IPC interface and spin up a background thread to serve it. Or turn the process into a Windows service (:see_no_evil:). No free lunch here.

@emersonknapp looking at the metadata itself, it looks like half of it we could write out at the beginning and the other half can be retrieved from storage (based on the details contained in the first half). It's a big change, but we'd get rid of this issue entirely.

[emersonknapp]

Yeah, the metadata as-is is a bit unwieldy, and was built with some undocumented use cases in mind (not by me so I'm only passing familiar with them).

We wouldn't be able to entirely write out the "first half" metadata at startup, because the most important bit is the relative file paths - this is only known on each recording split and so the metadata would need to be updated at those times. However that that would probably be fine, writing a YAML file is cheap. Much of the metadata indeed could be got from storage - but there is some value in the higher level metadata having the aggregated values across all subfiles, which an individual subfile would not be able to provide.

Just thinking out loud above - I'd be willing to review a proposal for a more crash-resistant metadata approach. Ideally one that doesn't change the end result very much.

[hidmic]

Just thinking out loud above - I'd be willing to review a proposal for a more crash-resistant metadata approach. Ideally one that doesn't change the end result very much.

Hmm, how about a rosbag2_transport::Recorder node that implements the lifecycle protocol? We could stop the recording before terminating the process. Parameter based configuration would be a prerequisite (#902).

[emersonknapp]

A slightly simpler approach might be to add a Recorder::stop method that can be called at will, instead of having to call the destructor for cleanup.

Parameter based configuration would be very nice but will be a pretty big effort, may not want to block this on that.

[hidmic]

A slightly simpler approach might be to add a Recorder::stop method that can be called at will, instead of having to call the destructor for cleanup.

Exposed as a service? That would work too. A bit ad-hoc, but yeah, simpler than parameter based configuration.