fix(config): pin every zenoh override against rmw_zenoh_cpp - #329
Open
YuanYuYuan wants to merge 4 commits into
Open
fix(config): pin every zenoh override against rmw_zenoh_cpp#329YuanYuYuan wants to merge 4 commits into
YuanYuYuan wants to merge 4 commits into
Conversation
crates/hiroz/src/config.rs claims to generate "rmw_zenoh_cpp compatible configs". Nothing checked it. Auditing all 31 overrides against upstream at e95c62df287143f78bdb41c452b6bf1e257b0c0d found one that did not hold, and it was recorded nowhere. queries_default_timeout was 60000 where upstream is 600000. Upstream raised it to ten minutes in cf09e854c9df17e0eb7e80ce4ab00e1b122a64e0 for slow service servers at launch -- the case hiroz's own comment cites while setting a tenth of it. docs/user-guide/config-advanced.md already published 600000, so the docs and the code disagreed. Realigned, and every override now matches. The test is not an equality check, and the reason is worth stating: hiroz's session listen/endpoints matched upstream exactly and was still wrong, because upstream's loopback locator leans on zenoh 1.8 router relaying that zenoh 1.9 withdrew. So it asserts instead that every difference is listed with a reason, and that every listed reason still describes a real difference -- a stale allow-list entry fails too. DIVERGENCES is empty today; the listen/endpoints fix is a separate change and must add its own entry, which this test forces. The reference is a vendored copy of upstream's two json5 files, byte-identical across all five rmw_zenoh distro branches. Where AMENT_PREFIX_PATH names an installed rmw_zenoh_cpp, the same test reads that instead, so the ROS legs also catch the vendored copy going stale. No workflow change: those legs already run hiroz-tests after sourcing setup.bash. AMENT_PREFIX_PATH rather than /opt/ros/$ROS_DISTRO because this workspace also builds ROS from Nix, where no /opt/ros exists. A second test fails if rmw_zenoh_cpp is installed but was not selected, so that fallback cannot go silent. crates/rmw-zenoh-rs/config/DEFAULT_RMW_ZENOH_SESSION_CONFIG.json5 is deleted. Nothing read it, and it had drifted far enough to mislead: multicast enabled, queries_default_timeout 10000, no listen block at all.
docs/core-concepts/services.md said a call with no server "times out after queries_default_timeout (default: 10 min)", and advised lowering that setting for faster failure detection. Neither is true of a ZClient. create_client hard-codes querier_timeout to Duration::from_secs(10) (node.rs), and ZClientBuilder::build passes it to .timeout() on the zenoh querier (service.rs), which takes precedence over the session default. That is the only construction site, and with_querier_timeout is pub(crate) -- so 10s is the effective figure and no public API changes it. The one caller of with_querier_timeout is the action client, which sets Duration::MAX and so has no deadline at all. Corrects the sequence diagram, its accDescr, the flashcard, the settings-table row and the "Reduce Service Call Timeout" section, which is renamed because it does not reduce a service call timeout. Each now says what the setting does reach and what it does not. Per .claude/rules/docs-consistency.md a promise the code does not keep is a defect, so this ships with the realignment rather than after it. Making the 10s figure configurable is a feature and is not in this change.
|
The file is not unread. crates/rmw-zenoh-rs/CMakeLists.txt installs it:
install(
DIRECTORY config
DESTINATION share/${PROJECT_NAME}
)
so deleting it made ament_cmake_symlink_install_directory fail with "can't
find .../crates/rmw-zenoh-rs/config" and took the rmw_zenoh_rs package down.
The check that missed it searched *.rs, *.toml, *.nu, *.sh and *.yml for the
file name. CMakeLists.txt was outside that list, and the reference is to the
DIRECTORY -- so no search for the file name would have found it at any breadth.
An empty search result is not evidence of no consumer.
The staleness that prompted the deletion is real and is left in place: the
installed copy still says multicast enabled and queries_default_timeout 10000,
where session_overrides() applies neither. Regenerating it from config.rs is
the fix, and it is a different change from this one.
There was a problem hiding this comment.
Pull request overview
Aligns hiroz’s Zenoh overrides with rmw_zenoh_cpp and adds regression coverage for future configuration drift.
Changes:
- Raises the session query timeout to 10 minutes.
- Adds vendored upstream configurations and alignment tests.
- Corrects service-timeout documentation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
docs/user-guide/config-advanced.md |
Clarifies raw-session query timeouts. |
docs/core-concepts/services.md |
Documents client timeout behavior. |
crates/hiroz/src/config.rs |
Sets the session query timeout to 600000 ms. |
crates/hiroz-tests/tests/data/rmw_zenoh_cpp/DEFAULT_RMW_ZENOH_SESSION_CONFIG.json5 |
Vendors upstream session configuration. |
crates/hiroz-tests/tests/data/rmw_zenoh_cpp/DEFAULT_RMW_ZENOH_ROUTER_CONFIG.json5 |
Vendors upstream router configuration. |
crates/hiroz-tests/tests/config_upstream_alignment.rs |
Tests override alignment and documented divergences. |
crates/hiroz-tests/Cargo.toml |
Adds JSON5 parsing support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… claims Three findings from review, each verified against source before the fix. A DIVERGENCES entry with a blank reason exempted drift while explaining nothing, because divergence_reason used is_some(). The reason is what makes an entry a decision rather than an exemption, so an entry without one no longer counts. A named check reports it, instead of the misleading "now agree on it". The settings table gave the router a 10-minute query timeout. hiroz sets queries_default_timeout only in session_specific_overrides, so the router keeps zenoh's 10 seconds and diverges from rmw_zenoh_cpp, which sets 10 minutes on both. The table now says 10 s, and the note below it states the divergence rather than claiming the configs match exactly. The services page said action clients set no deadline. Only the result client does: action/client.rs applies Duration::MAX to that builder alone, and the goal and cancel clients keep the 10-second default. Submitting and cancelling a goal both time out. The router divergence also exposes a limit of the test, now stated in its module doc: it compares the keys hiroz overrides, so a key upstream sets and hiroz leaves at zenoh's default is invisible to it. The module doc drops its branch-history narration and follows the STE sentence rules, as do the remaining comments.
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
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.
Summary
crates/hiroz/src/config.rssays it generates "rmw_zenoh_cpp compatible configs". Nothing checked that. This PR adds the check, fixes the one drift it found, and corrects three documentation claims about timeouts.rmw_zenoh_cppThe drift:
queries_default_timeoutwas60000where upstream is600000. Upstream raised it incf09e854for slow service servers at launch.docs/user-guide/config-advanced.mdalready published600000, so the docs and the code disagreed.Why this is not an equality check
Matching upstream is not the same as being correct.
hiroz's session
listen/endpointsmatchedrmw_zenoh_cppexactly. Two nodes on two hosts still delivered nothing to each other (#325). Upstream's loopback locator depends on zenoh 1.8 router relaying, and zenoh 1.9 removed it. hiroz runs 1.9. Upstream does not.So the test asserts something weaker and more useful:
DIVERGENCESgives a non-blank reasonconfig.rsDIVERGENCESlists a key that now matchesDIVERGENCESentry has a blank reasonRows four and five matter as much as row three. A stale or blank entry reads as a considered decision. It exempts the key anyway.
Important
DIVERGENCESis empty, which describesmaintoday. #325 creates the first real divergence, so #325 must add its own entry and this test fails until it does. Merge this first, then rebase #325 onto it.What it reads
The reference is a vendored copy of upstream's two json5 files. They are byte-identical across all five
rmw_zenohbranches, so one copy serves every distro.ROS-independent TestInterop tests with ROS 2 *(×4)rmw_zenoh_cppThis PR changes no workflow. Those legs already run
hiroz-testsafter sourcingsetup.bash.The lookup uses
AMENT_PREFIX_PATH, not/opt/ros/$ROS_DISTRO. The second is Debian's layout, and this workspace also builds ROS from Nix. A second test fails ifrmw_zenoh_cppis installed and the alignment test did not select it, so the fallback cannot go silent.What it does not cover
The test compares only the keys hiroz overrides. A key that upstream sets and hiroz leaves at zenoh's default is invisible to it.
One such key exists, and the review found it:
rmw_zenoh_cppsetsqueries_default_timeoutto 10 minutes on the router, and hiroz keeps zenoh's 10 seconds. The settings table now says so. Covering that direction needs a second allow-list, because most remaining upstream settings are moot here — multicast keys under disabled multicast, and keys zenoh 1.9 deprecated. That is a separate change.Commit 2 — the documented timeout is not the one that fires
docs/core-concepts/services.mdsaid a call with no server times out afterqueries_default_timeout, at 10 minutes. It does not.create_clienthard-codesquerier_timeoutto 10 snode.rs.timeout(), which overrides the session defaultservice.rswith_querier_timeoutispub(crate)Duration::MAXA service call times out after 10 seconds. Lowering
queries_default_timeoutdoes not change it.Review findings, all fixed
DIVERGENCESreason still exempted driftdivergence_reasonrejects a blank reason, and a named check reports itWhat fails without this
Measured on one worker. Each red case is a single-line edit, reverted before the next.
queries_default_timeoutback to60000is 60000 in hiroz and 600000 in rmw_zenoh_cppDIVERGENCESentry for a key that matchesnow agree on it. Remove the entry.no such keyis 600000 in hiroz and 12345 in rmw_zenoh_cppThe last two rows exist because the first three would all pass if the test ignored its reference. Mutating the vendored file proves those bytes are load-bearing.
The test also asserts it compared at least 25 overrides. Zero comparisons and zero failures otherwise look alike.
One thing this PR does not do
crates/rmw-zenoh-rs/config/DEFAULT_RMW_ZENOH_SESSION_CONFIG.json5is stale: it saysmulticast.enabled: truewheresession_overrides()setsfalse, andqueries_default_timeout: 10000. It ships toshare/rmw_zenoh_rs/config/, so it can mislead a reader.An earlier revision of this branch deleted it, and CI caught that:
crates/rmw-zenoh-rs/CMakeLists.txtinstalls the directory, soament_cmake_symlink_install_directoryfailed. Reverted ina218346. Regenerating the file fromconfig.rsis the fix, and is a separate change.Breaking changes
ZContext::session()and issuing its ownget().timeout(), or overridequeries_default_timeoutThis changes no hiroz API path. Every query hiroz issues sets its own timeout:
service.rs10 s,node.rs10 s,graph.rs3 s, action result clientDuration::MAX.