fix: don't park the ctrl_lock holder on StartConditions notifications under peer churn - #2637
Conversation
… to avoid ctrl_lock deadlock
Two peer sessions that discover each other over multicast and then open and close repeatedly wedge forever inside zenoh's session close. It is not theoretical: it is what made scope_test_panels time out at random under `ctest -j8`, and it reproduces in a 20-line program that uses zenoh and nothing of ours -- one process always finishes, two hang within a handful of cycles and never recover. The wait is in `Runtime::close_inner`, on its first await, found by instrumenting the two lines rather than by reading them: `terminate_all_async()` waits for every tracked task with no timeout, and gossip's autoconnect task is spawned with `spawn` rather than `spawn_abortable`, so the cancellation token never reaches it and it sits in `connect_peer()` against a peer that is itself shutting down. `spawn`'s own documentation requires such a task to be cancellable or to finish in finite time; connecting to a remote peer is neither, and the multicast scouting side already uses `spawn_abortable` for the same work. The patch is one word. patches/zenoh_abortable_gossip_connect.patch carries the full analysis, including why eclipse-zenoh/zenoh#2637 -- the obvious candidate, same file, same family -- was applied in full, measured, and still hung 6 runs out of 8. GETTING IT INTO THE BUILD IS THE AWKWARD PART, because CMake does not fetch the crate that contains the bug: cargo does, as a git dependency of zenoh-c. So we fetch that crate ourselves, patch it, and point cargo at the result with a `paths` override. The obvious alternative -- `cargo fetch` after zenoh-c is populated, then patch the checkout it downloaded, needing no second clone and no pinned revision -- was built and measured, and does not work: cargo treats a git dependency's source as immutable and fingerprints it by revision rather than mtime, so a patched checkout it has already compiled is ignored and the build silently links the cached, unpatched artifact. It appears to work exactly once, on a machine that has never built that revision. Three guards are in third_party/zenoh-c.cmake, each for a failure actually hit while building this: the branch is verified against the pinned sha, the patched source is grepped rather than trusting `git apply`'s exit code, and the cargo flag is added idempotently. Measured here, two processes, 50 cycles each: unpatched hung 3 of 3 runs; patched passed 10 of 10. Eight parallel scope_test_panels with peer discovery left ON, which used to hang 40 of 40, now all exit. THE ISOLATION FROM f1a59ea STAYS. It is no longer load bearing for the hang -- the check above says so -- but it was never only about the hang: test processes that find each other also SHARE A BUS, so one test's samples arrive in another's subscriber, which the `net` label warned about before any of this. It also covers the window where someone bumps zenoh-c and has to drop this patch. This is a bandaid with an end date. The same change, plus a regression test and the reproducer, is submitted upstream from github.com/ryandavid/zenoh, branch fix/abortable-gossip-autoconnect. When it lands in a zenoh-c release we take, delete the patch and the block in third_party/zenoh-c.cmake and bump the tag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E5MQBYNxf5GPSguyuJvRYk
Independent Reproduction (C API) & Safety Argument for Ordering ChangeWe identified this issue from the other end: our test suite experienced hangs lasting up to 300 seconds, initially reported against Below are our measurements and a safety analysis regarding the ordering change, which could be the primary concern for merging. Different Victim Path, Same DefectThe reproducer in #2581 stalls a publisher within Here, We observed identical behavior via Measurement DataWe ran
*The unpatched library was swapped back in on the same machine immediately after the patched runs. The immediate return of hangs confirms the variable is the library itself, not a transient improvement in system state. The variance in total wall time is driven by whether the 300s timeout triggers. The Switch to
|
|
Independent reproduction of this deadlock from a different workload (ROS 2 / Standalone reproducer: https://gist.github.com/otamachan/c43030eca21246d56151312fceb91d80 SetupROS 2 Jazzy, A router plus 40 resident peer sessions, then repeated waves of 30 new peer sessions joining the existing mesh, all pinned to 4 CPUs. Each "peer" is just a In ROS terms the symptom is that BacktraceFive threads, one cycle.
Full log is Two things that may be worth adding to the description:
A/BThe gist also documents how to build a patched 30 rounds each, one round = 30 fresh processes.
The control build rules out toolchain differences. Our production build also carries one unrelated local patch to the transmission pipeline; with that included the numbers are 8 / 30 before and 0 / 30 after this PR. Raising the Net worker count does not avoid it, so the single-worker default is what makes it easy to hit rather than what makes it possible:
Happy to run anything else against the reproducer if it helps move this along. |
Closes #2581
What happens
In peer mode with gossip autoconnect, the routing layer can deadlock permanently
when sessions open and close while other peers are connecting. A long-lived
publisher then stalls forever in
put().wait()(the symptom reported in #2581),and every other operation that touches routing (declares, accepts, OAM handling,
session close) wedges behind it. In debug builds the same race can instead
poison the routing
ctrl_lockand turn every rx callback into aPoisonErrorpanic cascade from
demux.rs.Root cause
Two call sites notify
StartConditionsthat a peer connector has terminated byrunning
terminate_peer_connector_zidthroughZRuntime::Net.block_in_place(..):hat::peer::interests::route_declare_final(initial-interest finalization),reached under the routing
ctrl_lockviaFace::send_declaregossip::link_states, reached under the routingctrl_lockviaOAM handling in
DeMux::handle_messageblock_in_placeparks the calling thread until the future completes, so in bothcases a thread holding
ctrl_lockgoes to sleep waiting on theStartConditions::peer_connectorstokio mutex. That closes a cycle, capturedlive with
gdb thread apply all bton a hung process:DeclareFinal, takesctrl_lockinFace::send_declare, and parks inblock_in_place(terminate_peer_connector_zid(..)), waiting on theStartConditionsmutex.worker_threads = 1by default) runs a gossipautoconnect task (
link_states->connect_peer->Gateway::new_transport_unicast) that blocks synchronously on the samectrl_lock.StartConditionsmutex arenever polled again, so the mutex never frees, the rx thread in (1) never
wakes, and
ctrl_lockis never released.Fix
Spawn the notification on the Net runtime instead of blocking on it.
terminate_peer_connector_zidreturns nothing and its only effect is to markthe connector terminated and possibly
notify_one()a waitingopen(), sodeferring it by one task-schedule is semantically equivalent. The call already
raced with
add_peer_connector_zidacross threads (terminate-before-add insertsan already-terminated entry), so the reordering introduces no new interleavings.
With no thread parked while holding
ctrl_lock, the cycle cannot form.Validation
Reproduced with a stress setup that repeatedly opens ~20 short-lived peer
sessions (declaring queryables and issuing queries) against a router while a
publisher streams at 5 kHz, all sessions gossip-linked. Unpatched, it deadlocked
within ~23 iterations on a 24-core Linux machine; patched, 150 iterations across
2/4/8/24-CPU affinities plus repeated full-suite runs completed with zero hangs
and zero panics. The reproducer from #2581 (
z_p2p_declare_final_stress)exercises the same churn pattern.
🏷️ Label-Based Checklist
Based on the labels applied to this PR, please complete these additional requirements:
Labels:
bug🐛 Bug Fix Requirements
Since this PR is labeled as a bug fix, please ensure:
Why this matters: Bugs without tests often reoccur.
Instructions:
- [ ]to- [x])This checklist updates automatically when labels change, but preserves your checked boxes.