feat: transport link rotation engine - #2746
Conversation
Add RotationConf, RotationPolicyConf, RotationModeConf, and RotationFallbackConf types to zenoh-config. Wire the rotation field into ConnectConfig with defaults (disabled by default). The initial policy is Interval-only; the enum is kept minimal with only Interval(Duration) to avoid dead code. Additional policies can be added as new variants when implemented. Rotation mode is MakeBeforeBreak only — BreakBeforeMake is not user-selectable, it is used internally as a fallback when make-before-break fails (to avoid redeclaration storms).
Add RotationEngine in zenoh/src/net/runtime/rotation.rs that periodically rotates transport links using a make-before-break strategy. The engine is spawned after a peer connection is established in spawn_peer_connector(). The rotation engine: - Opens a new link before closing the old one (make-before-break) - Retries up to fallback.max_retries times if make fails - Falls back to break-before-make only as a last resort - Uses configurable interval with random jitter The actual old-link closure mechanism requires a new TransportUnicast::close_link() API to be added to the transport layer. This is marked with TODO in the code.
Add rotation configuration section to the default config file with documentation for all fields: enabled, policy (interval), mode (make_before_break), fallback, and rotate_across_locators.
Comprehensive investigation covering: - Current architecture and session-transport decoupling - Connection lifecycle and reconnection flow - Proposed rotation design (make-before-break with fallback) - Configuration schema (RotationConf, RotationPolicyConf, etc.) - Implementation plan (4 phases) - Testing strategy (transport + session layer tests) - Edge cases, risks, and relationship to existing features
- Remove dead is_rotation_enabled() — fold into get_rotation_config()
- Use .then() instead of if/else for jitter calculation
- Use let-else for interval_ms match
- Use inline format args ({endpoint}) consistently
- Rename _cancellation_token field to clarify it's just for Drop
- Remove redundant comments and tighten code
Add close_link(Link) to the TransportUnicastTrait and expose it through the TransportUnicast public wrapper. This allows closing a specific link within a multilink transport without tearing down the entire transport. - TransportUnicastUniversal: delegates to existing del_link() - TransportUnicastLowlatency: closes the entire transport (it has at most one link) - MockTransportUnicastInner: no-op stub for tests
Replace the TODO placeholder with an actual call to TransportUnicast::close_link() to close the old link after the new one is established. This completes the make-before-break flow: open new link → close old link → transport survives.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2746 +/- ##
==========================================
- Coverage 74.67% 74.54% -0.14%
==========================================
Files 425 421 -4
Lines 63930 63984 +54
==========================================
- Hits 47739 47694 -45
- Misses 16191 16290 +99 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
(I haven't reviewed the code; but I have some questions on the design. I will however throw Copilot at the diff.)
[..] enabling cloud-friendly connection cycling.
Could you please elaborate on this, perhaps with a concrete use-case?
Fallback to break-before-make: only if make fails after all retries, as a last resort
Why should we assume that a given connection attempt would fail because of make-before-break?
If we know a priori that make-before-break is incompatible with a certain endpoint, then why go through retry loops? If we cannot know a priori that break-before-make is the solution to connection failure, it seems wrong to speculate and risk destroying the existing while still being unable to re-connect.
Full investigation and design rationale in docs/transport_rotation_investigation.md
I don't think this document fits within the codebase, perhaps it is better supplied as a file attachment to the pull request description. I understand that it's perhaps useful agentic context, but this codebase is yet to even gain an AGENTS.md file :)
There was a problem hiding this comment.
Pull request overview
Adds a transport link rotation feature to zenoh’s runtime/orchestrator to periodically cycle unicast connections (intended make-before-break), exposing the required transport API and configuration surface so deployments can proactively refresh cloud/LB/DNS-facing connections.
Changes:
- Introduces a per-endpoint rotation engine (
RotationEngine) and hooks it into peer connection orchestration. - Extends transport unicast API with
close_link(Link)to support targeted link teardown without necessarily closing the whole transport. - Adds rotation configuration types and documents the new config schema + investigation/design rationale.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
zenoh/src/net/runtime/rotation.rs |
New rotation engine (timer + make-before-break + fallback logic). |
zenoh/src/net/runtime/orchestrator.rs |
Starts rotation engine after successful peer connection. |
zenoh/src/net/runtime/mod.rs |
Exposes the new rotation runtime module. |
io/zenoh-transport/src/unicast/universal/transport.rs |
Implements close_link() for universal unicast transport via existing del_link(). |
io/zenoh-transport/src/unicast/transport_unicast_inner.rs |
Adds close_link(Link) to the internal unicast transport trait. |
io/zenoh-transport/src/unicast/test_helpers.rs |
Updates mock transport to satisfy new trait method. |
io/zenoh-transport/src/unicast/mod.rs |
Exposes TransportUnicast::close_link() public wrapper. |
io/zenoh-transport/src/unicast/lowlatency/transport.rs |
Implements close_link() for lowlatency transport by closing the transport. |
commons/zenoh-config/src/rotation.rs |
Adds rotation configuration types (RotationConf, policy/mode/fallback). |
commons/zenoh-config/src/lib.rs |
Wires rotation config into ConnectConfig and re-exports module. |
commons/zenoh-config/src/defaults.rs |
Adds default constants and initializes ConnectConfig.rotation. |
DEFAULT_CONFIG.json5 |
Documents rotation configuration and defaults. |
docs/transport_rotation_investigation.md |
Adds design/investigation document for transport rotation. |
Suppressed comments (1)
zenoh/src/net/runtime/rotation.rs:218
- Same locator-matching issue as in
try_make_before_break:links.iter().any(|l| l.dst == locator)can fail whenLink.dstcontains patched metadata (reliability/priorities), so the fallback may not find/close the intended transport.
let locator = endpoint.to_locator();
for transport in transports {
if let Ok(links) = transport.get_links() {
if links.iter().any(|l| l.dst == locator) {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| loop { | ||
| let jitter = (jitter_ms > 0) | ||
| .then(|| Duration::from_millis(rand::thread_rng().gen_range(0..=jitter_ms))) | ||
| .unwrap_or(Duration::ZERO); | ||
|
|
||
| tokio::select! { | ||
| _ = tokio::time::sleep(base_interval + jitter) => {} | ||
| _ = cancellation_token.cancelled() => { | ||
| tracing::debug!("Rotation engine for {endpoint} cancelled."); | ||
| return; | ||
| } | ||
| } |
| let links = new_transport.get_links().unwrap_or_default(); | ||
| if links.len() > 1 { | ||
| let locator = endpoint.to_locator(); | ||
| let old_links: Vec<_> = links.into_iter().filter(|l| l.dst == locator).collect(); | ||
| for old_link in old_links.iter().take(old_links.len().saturating_sub(1)) { |
| // Start rotation engine if configured | ||
| if let Some(rot_conf) = rotation_conf { | ||
| tracing::info!( | ||
| "Starting rotation engine for {} with interval {:?}", | ||
| peer, |
Two concrete cloud-related use-cases:
While UDP and QUIC could be an approach they are still not well supported in Cloud infrastructures.
Because of the
|
All very reasonable.
Especially since zenoh/commons/zenoh-protocol/src/transport/close.rs Lines 21 to 31 in c5d4760 |
Fixes based on review by fuzzypixelz and Copilot: 1. Remove investigation doc from repo (fuzzypixelz: doesn't fit in the codebase, better as PR attachment) 2. Fix jitter to be ±jitter_ms instead of always-positive delay. Previously the interval range was [base, base+jitter] (always slower than configured). Now it's [base-jitter, base+jitter] so clients can desynchronize in both directions. 3. Fix locator matching to compare protocol+address without metadata. Link.dst may have patched metadata (reliability/priorities) that endpoint.to_locator() does not carry, causing the comparison to fail and old links to accumulate up to max_links. 4. Fix fallback race: fallback_break_before_make no longer calls open_transport_unicast itself. It only closes the old transport, letting the orchestrator's closed_session() callback handle reconnection via peers_connector_retry(). This avoids duplicate concurrent connection attempts. 5. Remove retry loop from fallback. Per fuzzypixelz's suggestion: if make-before-break fails (typically because max_links=1 and the connection lands on the same router), retrying would likely hit the same limit. Instead, either skip the rotation (fallback disabled) or fall back directly to break-before-make (fallback enabled). The error is logged in both cases. 6. Remove RotationFallbackConf.max_retries and retry_backoff_ms fields, update DEFAULT_CONFIG.json5 and defaults.rs accordingly.
Summary
Implement transport link rotation: a middleware-level capability to automatically close and re-establish transport links on a configurable schedule, enabling cloud-friendly connection cycling.
Design
Changes
Config (commons/zenoh-config)
rotation.rsmodule with RotationConf, RotationPolicyConf, RotationModeConf, RotationFallbackConfRotation Engine (zenoh/src/net/runtime/rotation.rs)
Transport Layer
Configuration Example
Depends on
Investigation Document
Full investigation and design rationale in
docs/transport_rotation_investigation.md🏷️ Label-Based Checklist
No specific label requirements detected.
Current labels: No labels
Add one of these labels to this PR to see relevant checklist items:
api-sync,breaking-change,bug,ci,dependencies,documentation,enhancement,new feature,internalThis section updates automatically when labels change.