Skip to content

fix: make StartConditions sync, drop block_in_place under ctrl_lock - #2779

Open
YuanYuYuan wants to merge 1 commit into
eclipse-zenoh:mainfrom
YuanYuYuan:fix/ctrl-lock-block-in-place
Open

fix: make StartConditions sync, drop block_in_place under ctrl_lock#2779
YuanYuYuan wants to merge 1 commit into
eclipse-zenoh:mainfrom
YuanYuYuan:fix/ctrl-lock-block-in-place

Conversation

@YuanYuYuan

@YuanYuYuan YuanYuYuan commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #2581. Under peer churn, the RX thread can deadlock the whole session.

The RX thread holds ctrl_lock. It calls block_in_place to wait on StartConditions, which is a tokio::sync::Mutex. At the same time, the Net runtime's single worker thread tries to acquire that same ctrl_lock, from Router::new_transport_unicast during gossip autoconnect. The worker cannot run. So the task that would release StartConditions never runs either. The session freezes.

This PR is a companion to #2637. #2637 fixes the same root cause with a different mechanism. See "Relationship to #2637" below.

The deadlock

  • RX thread: holds ctrl_lock. Calls block_in_place on StartConditions. Two call sites: gossip.rs's link_states tail, and the peer hat's route_declare_final.
  • Net worker (the runtime's only worker thread): blocked separately, trying to acquire ctrl_lock from Router::new_transport_unicast during gossip autoconnect.
  • Nothing releases StartConditions, because the one worker that could run is stuck on ctrl_lock. The session wedges: puts freeze, transport leases die.

The fix

Every StartConditions critical section is a plain Vec push or drain. None of them await anything. So peer_connectors becomes a std::sync::Mutex instead of a tokio::sync::Mutex. Every method on StartConditions becomes synchronous. Both block_in_place call sites are deleted, not rescheduled.

Relationship to #2637

#2637 takes a different approach at the same two sites: spawn the notification as a task on ZRuntime::Net, instead of blocking on it. That removes the RX thread from this specific cycle. But the spawned task still needs ZRuntime::Net's one worker to run. Nothing in that fix adds a second worker, and nothing removes the single-worker precondition that caused the deadlock. If that worker is ever blocked elsewhere, the spawned task queues indefinitely. If a future caller blocks synchronously on the task's result, the same deadlock returns, with one extra hop.

This PR removes the scheduling dependency instead. There is no .await left inside StartConditions for anything to be rescheduled onto.

@otamachan's independent reproduction on #2637 goes through rmw_zenoh_cpp and ROS 2 directly, with a full five-thread backtrace. It confirms both #2637's approach and this PR's approach target the same two block_in_place sites. Worth reading alongside this PR.

Credit

This PR ports a fix independently developed and field-validated by Guillaume Doisy (Dexory): botsandus/zenoh@ff6cc2bc3. Authorship is preserved on the commit (Author: Guillaume Doisy <guillaume@dexory.com>). Their original commit message, quoted for the record:

Seen in production as random ROS 2 process freezes on 16-25% of bringups.

This PR ports that fix onto current main. It adapts it past the #2096 hat rename (p2p_peer to peer) and the resulting route_declare_final signature change. No other logic changed.

Testing

  • cargo fmt --check -p zenoh: clean
  • cargo clippy -p zenoh --lib -- -D warnings: clean. Five pre-existing warnings remain in unrelated files, untouched by this diff.
  • cargo test -p zenoh --lib net::runtime::orchestrator: 3 of 3 pass
  • No new test covers the deadlock itself. The field evidence cited above already covers it at a scale (about 1,700 restart cycles, plus an independent 200Hz A/B test) that a unit test here cannot easily reproduce. Happy to add a regression test if maintainers want one in a specific shape.

The RX thread handles OAM/Declare messages holding ctrl_lock and the
tables write lock, then parks in ZRuntime::Net.block_in_place(...)
waiting on the StartConditions tokio mutex (two sites: the gossip
link_states tail and the peer hat's route_declare_final). The single
Net worker meanwhile blocks on ctrl_lock in autoconnect's transport
setup, and tokio's fair mutex handoff can strand the permit in a Net
task that never gets polled again -- deadlocking the whole session.

Every StartConditions critical section is a pure Vec operation that
never awaits, so its tokio::sync::Mutex becomes std::sync::Mutex, all
its methods go sync, and both block_in_place sites are deleted
outright.

Ported onto current main from a fix independently developed and
field-validated by Guillaume Doisy (Dexory), adapting it past the
eclipse-zenoh#2096 hat rename (p2p_peer -> peer) and the resulting route_declare_final
signature change. Original work, full credit preserved via authorship.

See PR body for the upstream discussion this consolidates.
@YuanYuYuan YuanYuYuan added the bug Something isn't working label Sep 9, 2026
@YuanYuYuan
YuanYuYuan marked this pull request as ready for review September 9, 2026 11:02
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.76%. Comparing base (b828c6c) to head (49c2dd9).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
zenoh/src/net/protocol/gossip.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2779      +/-   ##
==========================================
+ Coverage   74.72%   74.76%   +0.04%     
==========================================
  Files         419      419              
  Lines       63955    63934      -21     
==========================================
+ Hits        47790    47803      +13     
+ Misses      16165    16131      -34     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@doisyg

doisyg commented Sep 9, 2026

Copy link
Copy Markdown

ECA validated

@YuanYuYuan
YuanYuYuan requested a review from JEnoch September 10, 2026 15:33
@YuanYuYuan

Copy link
Copy Markdown
Contributor Author

ECA validated

@doisyg Thanks!

@JEnoch Could you please review this?

@JEnoch JEnoch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I would like also @OlivierHecart to review, as he's the architect of this code part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Persistent routing lock stall/deadlock under peer churn

4 participants