Feature/rosbag2 circular logging size duration 2217

[luke-alloy]

Description

Adds circular logging by split count, record size and duration to rosbag2 recording.

• CLI: adds --max-splits
• Writer: deletes oldest files when exceeding split count

Fixes #2217

Is this user-facing behavior change?

Yes.

• Older recordings are deleted once max split is reached:
  • --max-splits N: limit number of retained bagfile splits.
• Examples:
  • ros2 bag record -a --max-bag-size 100MB --max-splits 5

Did you use Generative AI?

Yes.

• Tool/model: GPT-5
• Used for: naming suggestions, CLI/help text wording, and drafting PR description.
• Code impact: assisted in scaffolding changes (option wiring, param names); final code implementation was completed, built, run, and manually reviewed and tested locally.

Additional Information

• The max split limit is checked together after each bag split
• The writer logs a warning when deleting files due to limits. Adjust --log-level if needed.
• metadata.yaml currently retains aggregate message_count and topics_with_message_count across the entire capture, even when older bagfiles are pruned by circular logging. File lists and durations reflect pruning, but those two fields still represent the full session totals.
• Circular logging changes are only in sequential_writer.cpp (deleting the oldest file if needed); the rest involve adding StorageOptions parameters, updating the shell UI, and adjusting related unit tests.

[luke-alloy]

I've committed changes that should address the unit test error. I'm also happy to include @fujitatomoya ’s suggestion to remove --max-record-size and --max-record-duration, pending confirmation to proceed.

[MichaelOrlov]

@luke-alloy Thank you for your contribution. This is a long-time-wanted feature that will be very useful.

I've made a thorough review, and overall implementation looks good to me. However, I have a few suggestions:

1. The naming is hard. I would propose to rename --max-splits parameter to --max_bag_files to avoid confusion and multiple user questions, like:

• Will splits stop after reaching max_splits?
• Why is there one file when max_splits = 1? I would expect to have at least two files after the split.

2. I would like to see at least one integration test with real files writing and deletion.
   To facilitate this, we do have a special TemporaryDirectoryFixture. Please consider modifying your tests or adding another one. I recently added a similar test in one of my PR. Please, refer to the 81ef9fd#diff-c14a7d44179cf64868ce86b5e60c7f741f957ef368c04a0477ccfe7ccd89f1f2R138-R164 as an example from the #2224.

[MichaelOrlov]

Suggested change

	'Requires --max-bag-size or --max-bag-duration to be set.')
	'Requires --max-bag-size or --max-bag-duration to be set or usage of the bag split '
	'via direct recorder API or service calls.')

[MichaelOrlov]

This assignment would be better readable in one line

Suggested change

	storage_options_.uri = format_storage_uri(
	base_folder_,
	metadata_.relative_file_paths.size());
	next_file_index_);
	storage_options_.uri = format_storage_uri(base_folder_, next_file_index_);

[MichaelOrlov]

Suggested change

	// Delete oldest files if circular buffer limit exceeded (after split creates new file)
	delete_oldest_files_if_needed();

I think it would be better to move this delete_oldest_files call into the void SequentialWriter::switch_to_next_storage() right before storage_->update_metadata(metadata_);. This way we can avoid extra storage_->update_metadata(metadata_); call inside delete_oldest_files_if_needed();
Please be aware that our update_metadata(metadata_) in mcap is a bit suboptimal, and each time update is called, we are writing a new section with metadata instead of really updating the one.

[MichaelOrlov]

Suggested change

	// Delete the oldest files if circular buffer limit exceeded (after split creates new file)
	delete_oldest_files_if_needed();
	storage_->update_metadata(metadata_);

"Moved from after the split"

[MichaelOrlov]

Suggested change

	// Delete oldest files until we're under the split-count limit
	// Note: We just split, so we have at least 2 files and the oldest is definitely not current
	while (metadata_.files.size() > 1) {
	// If we're under the limit, stop deleting
	if (metadata_.files.size() <= storage_options_.max_splits) {
	break;
	}
	// Delete the oldest files until we're under the split-count limit
	while (metadata_.files.size() <= storage_options_.max_splits) {

Simplify logic.
Note: if storage_options_.max_splits were storage_options_.max_bag_files, the logic would be easy to understand.

[MichaelOrlov]

Suggested change

	// Update metadata after deletions
	if (!metadata_.files.empty()) {
	storage_->update_metadata(metadata_);
	}

Metadata update should happen once in the SequentialWriter::switch_to_next_storage()

[MichaelOrlov]

Suggested change

	// This feature is only available if either max_bagfile_size or max_bagfile_duration is set.
	// This feature is only available when the bag split is active.
	// Requires --max-bag-size or --max-bag-duration to be set or usage of the bag split via
	// direct recorder API or service calls.set.

[MichaelOrlov]

Suggested change

	uint64_t max_splits = 0;
	uint64_t max_bag_files = 0;

The naming is hard. I would propose to rename this parameter to max_bag_files to avoid confusion and multiple user questions, like:

• Will splits stop after reaching max_splits?
• Why is there one file when max_splits = 1? I would expect to have at least two files after the split.

[MichaelOrlov]

Suggested change

The stray empty line should be removed.

[luke-alloy]

@luke-alloy Thank you for your contribution. This is a long-time-wanted feature that will be very useful.

I've made a thorough review, and overall implementation looks good to me. However, I have a few suggestions:

1. The naming is hard. I would propose to rename --max-splits parameter to --max_bag_files to avoid confusion and multiple user questions, like:

• Will splits stop after reaching max_splits?
• Why is there one file when max_splits = 1? I would expect to have at least two files after the split.

2. I would like to see at least one integration test with real files writing and deletion.
   To facilitate this, we do have a special TemporaryDirectoryFixture. Please consider modifying your tests or adding another one. I recently added a similar test in one of my PR. Please, refer to the 81ef9fd#diff-c14a7d44179cf64868ce86b5e60c7f741f957ef368c04a0477ccfe7ccd89f1f2R138-R164 as an example from the Fix for C++ Recorder failure on stop() -> record() due to reusing the bag name #2224.

@MichaelOrlov Thanks for the detailed review! I will update the PR soon.

I completely agree regarding the naming confusion—good catch. To maintain consistency with the other CLI flags, would --max-bag-files (using hyphens instead of _) be the preferred convention?

[MichaelOrlov]

@luke-alloy Yes. Need to use --max-bag-files (using hyphens instead of _).
Sorry for the confusion. It was my mistake, I copied and pasted the name from another post where I was referring to the storage_options