Repository navigation
Don't truncate rmw_time_t QoS durations in the override defaults - #3268
Open
Dev-next-gen wants to merge 2 commits into
Open
Dev-next-gen wants to merge 2 commits into
Dev-next-gen wants to merge 2 commits into
Conversation
fujitatomoya
requested changes
Sep 13, 2026
fujitatomoya
left a comment
fujitatomoya
left a comment
Collaborator
There was a problem hiding this comment.
I could not build the full ROS 2 stack here
why? can you take a look at https://docs.ros.org/en/rolling/Get-Started/Installation/Alternatives/Ubuntu-Development-Setup.html
IMO this looks good, but we are not going to approve that is NOT verified, we are not sure if this can be built without error.
Collaborator
|
and please use our pull request template. |
Author
|
Thanks, that was a fair objection — I should not have opened it unverified. I built and tested it. Official With the change, which is the truncated value the description predicts. The other seven tests pass on both sides. Full output is in the updated description. I also rewrote the description using the ros2 template, including the Generative AI section. |
Dev-next-gen
force-pushed
the
fix/qos-override-default-rmw-duration-narrowing
branch
from
September 14, 2026 01:15
df79610 to
ec6a61d
Compare
Collaborator
|
Pulls: #3268
|
Author
|
Thanks for kicking off the full CI, appreciated. Quick recap of where this stands so it's easy to pick back up: the change is verified on my side (official ros:rolling image, It's been a couple of weeks, so if it helps I'm happy to rebase on current rolling to refresh the branch and let the CI re-run cleanly — just say the word. And if anything in your ci.ros2.org run needs addressing, point me at it and I'll take care of it. Found by a defect-hunting pipeline I build and run (Dev-next-gen), using Claude Code with Anthropic's Claude Opus 5. |
Collaborator
|
@Mergifyio rebase |
Contributor
❌ This pull request comes from a fork and cannot be rebasedDetailsGitHub refuses an OAuth token on its rebase API for a fork, so rebasing one means impersonating a GitHub user to force-push the contributor's branch. Mergify does not do that. Use the |
Collaborator
|
@Dev-next-gen can you rebase against the rolling? the thing is that, we removed some package dependencies recently, so without rebasing, it will fail to build. i need to rerun the whole ci after rebase. |
rclcpp::detail::rmw_duration_to_int64_t built an rclcpp::Duration from static_cast<int32_t>(rmw_duration.sec). RMW_DURATION_INFINITE, and the best-available deadline and liveliness lease sentinels, all carry 9223372036 seconds, so the cast wrapped and the helper returned 633437444854775807 ns instead of 9223372036854775807 ns. That value is what declare_qos_parameters() declares as the default for the deadline, lifespan and liveliness_lease_duration overrides, and it is fed straight back into the profile through apply_qos_override(). Enabling those overrides on a profile carrying an infinite duration therefore silently replaced it with a finite ~20 year duration, which changes QoS compatibility matching. Seconds values in [2^31, 2^32) wrap to a negative int32_t and produce a negative duration, which QoS::deadline() then rejects with "rmw_time_t cannot be negative". Use rclcpp::Duration::from_rmw_time(), which is the conversion the QoS getters already use and which saturates instead of wrapping. Signed-off-by: leoca <leo.camus23@gmail.com>
Signed-off-by: leoca <leo.camus23@gmail.com>
Dev-next-gen
force-pushed
the
fix/qos-override-default-rmw-duration-narrowing
branch
from
October 7, 2026 00:15
ec6a61d to
32ae0e7
Compare
Author
|
@fujitatomoya I will do |
Author
|
Rebased onto current One honest caveat: I did not rebuild after this rebase on my side, so the CI run you mentioned is the real check here. Ready whenever you want to relaunch it. Found by a defect-hunting pipeline I build and run (Dev-next-gen), using Claude Code with Anthropic's Claude Opus 5. |
Collaborator
|
Pulls: #3268
|
ABI Compliance Check❌ Verdict: incompatible
❌
|
Author
|
The ABI workflow is configured right for this case: I saved the report body posted here and the one posted on #3299, normalised only the commit sha each one carries, and got the same file: #3299 changes exactly one file, What I have not worked out is why the base build and the head build disagree on those COMDAT symbols to begin with, so I can't give you the cause — only the fact that the identical disagreement shows up on a PR that cannot change the ABI at all. Found by a defect-hunting pipeline I build and run (Dev-next-gen), using Claude Code with Anthropic's Claude Opus 5. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
rclcpp::detail::rmw_duration_to_int64_t()converts anrmw_time_tby hand:rmw_time_t::secis auint64_t, and the durations that matter here do not fit in anint32_t.RMW_DURATION_INFINITEis{9223372036, 854775807}, andRMW_QOS_DEADLINE_BEST_AVAILABLE/RMW_QOS_LIVELINESS_LEASE_DURATION_BEST_AVAILABLEare{9223372036, 854775806}. Casting9223372036toint32_twraps to633437444, so the helper returns633437444854775807ns where it should return9223372036854775807ns.declare_qos_parameters()uses that value twice: as the default for thedeadline,lifespanandliveliness_lease_durationoverride parameters, and then it feeds the declared value back into the profile throughapply_qos_override(). So enabling those overrides on a profile carrying an infinite duration silently turns it into a finite one of about 20 years, which is a different thing for QoS compatibility matching — a subscription requesting a 20 year deadline no longer matches a publisher offering an infinite one. Seconds in[2^31, 2^32)are worse: they wrap negative, produce a negativeDuration, andQoS::deadline()throwsrmw_time_t cannot be negative.rclcpp::Duration::from_rmw_time()already does this conversion correctly — it saturates atINT64_MAXinstead of wrapping — and it is what theQoS::deadline(),QoS::lifespan()andQoS::liveliness_lease_duration()getters use. This PR makes the helper use it too.For every
rmw_time_twhose seconds fit in anint32_tthe two expressions produce the same number, so nothing changes for ordinary durations. The function isinlineand lives inrclcpp::detail, so no public signature moves.Fixes # (no issue filed; found while reading the QoS override code)
Is this user-facing behavior change?
Yes, for one case that is currently broken. A QoS profile with an infinite
deadline,lifespanorliveliness_lease_durationkeeps that infinite value when the corresponding parameter override is declared, instead of being silently reduced to ~292 years... in practice633437444854775807ns, about 20 years. Profiles whose durations already fit in anint32_tnumber of seconds are unchanged.Did you use Generative AI?
Yes. Claude Opus 5, through Claude Code, found the truncation, wrote the one-line change in
qos_parameters.hpp, wrote thedeclare_infinite_durationstest intest_qos_parameters.cpp, and drafted this description. The build and test verification below was run afterwards and is reported exactly as the container printed it.Additional Information
Verification. My earlier note said I could not build the full ROS 2 stack. That is now done, in the official
ros:rolling-ros-baseimage with the test dependencies installed byrosdep. Environment: Ubuntu 26.04.1, ROS 2 Rolling, gcc 15.2.0, cmake 4.2.3. Base:e47c084onrolling.With this change applied,
colcon build --packages-select rclcppsucceeds and the test file passes:Then I reverted only
rclcpp/include/rclcpp/detail/qos_parameters.hpp, keeping the new test, rebuilt, and it fails on currentrolling:That is the truncated value predicted above, produced by the code as it stands today. The other seven tests in the file pass on both sides.