fix(liveliness): decouple send_interest replay from calling thread#2678
Open
YuanYuYuan wants to merge 24 commits into
Open
fix(liveliness): decouple send_interest replay from calling thread#2678YuanYuYuan wants to merge 24 commits into
YuanYuYuan wants to merge 24 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2678 +/- ##
==========================================
- Coverage 74.31% 74.30% -0.01%
==========================================
Files 414 414
Lines 62301 62306 +5
==========================================
Hits 46299 46299
- Misses 16002 16007 +5 ☔ View full report in Codecov by Harness. |
13 tasks
YuanYuYuan
force-pushed
the
dev/liveliness-query-deadlock
branch
from
July 16, 2026 10:39
b0b11de to
c255693
Compare
Confirms the same self-declare/self-query topology completes cleanly once the FifoChannel capacity exceeds the token count, isolating the root cause to synchronous-replay-size vs. channel-capacity rather than anything inherent to the self-declare/self-query shape.
Dropping tokens/session and joining the background thread happened as part of detecting the query's outcome, so their own (possibly slow or hanging) cleanup was silently conflated with the query hang under test. Leak session/tokens/thread once the query result is known so teardown cost can never masquerade as (or mask) a real deadlock.
Each candidate_a test deliberately leaks its session/tokens/thread to keep query-hang detection uncontaminated by teardown cost. Running all four as #[test]s in one process meant leaked state from earlier tests could bleed into later ones -- confirmed directly: the router-mode oversized-handler control hung when run after 3 other tests in the same process, but passed cleanly (0.53s) in isolation. Cargo treats each direct tests/*.rs file as its own binary/process, so splitting into four single-test files gives real process isolation without needing nextest or manual subprocess forking. Shared setup lives in tests/liveliness_self_deadlock_support/mod.rs, which cargo's default tests/*.rs discovery does not pick up as its own binary.
Session::liveliness_query synchronously replayed every matching token onto the query's reply channel on the calling thread before returning, so a bounded FIFO handler self-deadlocked once the token count exceeded its capacity. Offload the replay via spawn_abortable_with_rt + spawn_blocking so the caller can return and start draining concurrently. spawn_blocking (rather than running inline on the async task) avoids a second regression: send_declare can invoke arbitrary blocking user callbacks, which would otherwise starve ZRuntime::Net's small worker pool and stall Session::close_inner's own teardown.
The spawn_blocking result was silently dropped, so a panic inside send_declare would vanish instead of being visible anywhere. Also trim the comment explaining the ZRuntime::Net-vs-blocking-pool rationale down to what's load-bearing.
Per review: the shared support/mod.rs added an extra file for a helper only two tests used, and running them in the same process (rather than separate cargo test binaries) previously caused real cross-test contamination. Inline the ~90-line helper into each test file directly instead, keeping the two files self-contained and process-isolated without a third module file.
Face::send_interest's local-face replay now runs on a spawned task (the liveliness-deadlock fix) rather than synchronously, but tests in src/net/tests/regions use MockFace::interest[_wildcard] and then immediately assert on the recorder synchronously. Give the replay a moment to land before returning to the caller. Also fixes rustfmt (import grouping, assert_eq wrapping) flagged by CI on the two liveliness_self_deadlock test files.
liveliness_deadlock_repro.rs was leftover exploratory code from an
early, never-confirmed investigation branch (candidate B, direct-link
two-session repro) -- superseded by the candidate-a-based regression
tests that actually ship. It was also never rustfmt-clean, which was
failing CI. Drop it; the confirmed regression coverage lives in
liveliness_self_deadlock_{peer,router}.rs.
Rewrite the manual loop{match{...break}} reply-drain as a while loop,
per clippy::while_let_loop (-D warnings in CI).
Missed one send_interest call site in the earlier fix -- the forwarding-test inject() helper injects Interest messages directly and its callers assert on recorded message counts immediately after.
three_node_combination consistently timed out on CI (0/3 nextest retries passing on both macos and ubuntu-shared-memory), stalled on LivelinessSub's initial declare_subscriber -- the one-time Current interest replay that now runs on a spawned task instead of synchronously. The recipe's 10s budget (shared TIMEOUT const, also used for MSG_COUNT=50 message delivery) was already tight with randomized per-node startup delays; the async replay's scheduling hop pushed it over. Double the budget to 20s.
20s still wasn't enough under the heavier unstable-feature test binary (836 tests vs 452 in the plain build, more concurrent load in the same process) -- all 3 nextest retries failed consistently, not marginally. Go to 45s for real headroom instead of inching up again.
CI observed failures at ~2x the prior 45s budget on macOS, ubuntu (shared memory), and windows (unstable), consistent with chained ztimeout! calls plus the recipe's own outer watchdog racing on the same shared constant under load.
…nterest replay Aborting the replay task on teardown drops its JoinHandle without stopping the nested spawn_blocking closure, which runs to completion regardless -- leaving Arc clones of session/face state alive past Session::close(). This raced dead_after_drop_many_times, which asserts all state is dropped immediately after close(). spawn_with_rt makes close()'s task_controller wait for genuine completion instead; the core deadlock fix's correctness does not depend on abort semantics.
YuanYuYuan
force-pushed
the
dev/liveliness-query-deadlock
branch
from
July 16, 2026 14:13
c255693 to
a1585ce
Compare
The prior fix widened the shared TIMEOUT const to 90s for every test in routing.rs, but only three_node_combination actually needs the extra headroom: its recipes exercise Face::send_interest's now-async declare replay, which queues on ZRuntime::Net's single worker thread and can occasionally exceed the old 10s budget under CI contention. Restore TIMEOUT to 10s and give three_node_combination its own THREE_NODE_COMBINATION_TIMEOUT via a new Recipe::run_with_timeout, leaving the deadline unchanged for gossip*, static_failover_brokering, two_node_combination, three_node_combination_multicast, and router_linkstate.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a self-deadlock in Session::liveliness_query/bounded FIFO reply handling by decoupling Face::send_interest’s “replay of currently-live declares” from the calling thread, so the caller can begin draining the reply channel concurrently instead of blocking inside the synchronous replay.
Changes:
- Dispatch
Face::send_interest’s declare replay fan-out ontoZRuntime::Netand run the potentially-blockingsend_declarecalls on the runtime’s blocking pool. - Add integration regression tests (
peer+router) that reproduce the pre-fix deadlock with >256 liveliness tokens and assert all replies are delivered. - Adjust test harnesses to account for the replay becoming asynchronous (targeted timeout widening + harness wait tweaks).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
zenoh/src/net/routing/dispatcher/face.rs |
Moves send_interest replay fan-out off the calling thread via runtime task + blocking pool. |
zenoh/tests/liveliness_self_deadlock_peer.rs |
New integration regression test for peer-mode self-declare/self-query deadlock. |
zenoh/tests/liveliness_self_deadlock_router.rs |
New integration regression test for router-mode self-declare/self-query deadlock. |
zenoh/src/net/tests/regions/mod.rs |
Updates mock routing harness behavior to accommodate async replay (currently via sleeps). |
zenoh/tests/routing.rs |
Adds per-recipe timeout override and widens timeout only for three_node_combination. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+685
to
+689
| // `send_interest`'s local-face replay (declares + DeclareFinal) runs on a | ||
| // spawned task now, not synchronously on this thread -- give it a moment | ||
| // to land before returning, since callers immediately inspect the | ||
| // recorder synchronously. | ||
| thread::sleep(Duration::from_millis(50)); |
Comment on lines
+699
to
+708
| ext_tstamp: None, | ||
| ext_nodeid: NodeIdType::DEFAULT, | ||
| }); | ||
| thread::sleep(Duration::from_millis(50)); |
Comment on lines
+1
to
+4
| //! Regression test: a peer-mode `Session` declares 300 liveliness tokens on | ||
| //! itself, then queries its own local table with the default 256-slot | ||
| //! handler -- the exact path that self-deadlocked pre-fix (see | ||
| //! KNOWLEDGE.md). Must complete promptly and deliver every reply. |
Comment on lines
+1
to
+4
| //! Regression test: a router-mode `Session` declares 300 liveliness tokens | ||
| //! on itself, then queries its own local table with the default 256-slot | ||
| //! handler -- the exact path that self-deadlocked pre-fix (see | ||
| //! KNOWLEDGE.md). Must complete promptly and deliver every reply. |
Comment on lines
+31
to
+35
| let mut config = Config::default(); | ||
| config.set_mode(Some(WhatAmI::Peer)).unwrap(); | ||
| // No network: no listen/connect endpoints configured. | ||
| let session = zenoh::open(config).await.unwrap(); | ||
|
|
Comment on lines
+31
to
+35
| let mut config = Config::default(); | ||
| config.set_mode(Some(WhatAmI::Router)).unwrap(); | ||
| // No network: no listen/connect endpoints configured. | ||
| let session = zenoh::open(config).await.unwrap(); | ||
|
|
Comment on lines
+559
to
+576
| self.state | ||
| .task_controller | ||
| .spawn_with_rt(zenoh_runtime::ZRuntime::Net, async move { | ||
| // `send_declare` may invoke a blocking user callback, which | ||
| // would otherwise starve ZRuntime::Net's small async-worker | ||
| // pool (shared with Session::close_inner's teardown). Run it | ||
| // on Net's separate blocking-thread pool instead. | ||
| if let Err(e) = zenoh_runtime::ZRuntime::Net | ||
| .spawn_blocking(move || { | ||
| for (p, m) in declares { | ||
| m.with_mut(|m| p.send_declare(m)); | ||
| } | ||
| }) | ||
| .await | ||
| { | ||
| tracing::error!(?e, "panic while replaying declares for send_interest"); | ||
| } | ||
| }); |
Comment on lines
+1111
to
+1115
| target.face.send_interest(&mut i.clone()); | ||
| // See the comment on `MockFace::interest` -- send_interest's | ||
| // local-face replay now runs on a spawned task, not | ||
| // synchronously; give it a moment to land before returning. | ||
| thread::sleep(Duration::from_millis(50)); |
- Replace fixed 50ms sleeps in the mock routing-test harness with a bounded poll on the DeclareFinal completion marker, so the wait is correctness-driven instead of timing-dependent. - Disable scouting (multicast + gossip) in the self-deadlock regression tests so they can't autoconnect to unrelated sessions from other test binaries running concurrently. - Drop the dead KNOWLEDGE.md reference (not part of this repo); restate the root cause inline instead. - Bind send_interest's spawned JoinHandle instead of discarding it silently, and distinguish a genuine panic from cancellation when logging a spawn_blocking failure.
rustfmt: reflow the spawn_with_rt call in face.rs to the formatter's preferred layout. nextest: net::tests::regions tests using MockFace::interest now wait on send_interest's replay, dispatched onto the shared, single-worker ZRuntime::Net instead of running synchronously. Under CI's parallel test execution that adds real scheduling latency that blew past the previous 2s/no-retry budget (several *_with_interest tests timed out consistently). Widen to 10s with one retry.
DeclareFinal is only sent when RouteInterestResult::ResolvedCurrentInterest (see dispatcher/interests.rs) -- interests requiring cross-region forwarding stay unresolved until the caller's own bi_fwd_all() pumps messages across the mock connections. The previous wait_for_declare_final asserted on a timeout, which deadlocked (by construction) any test whose interest needs multi-hop resolution: the code that would make forwarding progress can't run until the assert-wait returns, and the wait can't succeed until that code runs. Replace with settle_after_send_interest, a short (500ms) best-effort poll that returns silently if DeclareFinal never lands locally, instead of hard-blocking or panicking.
…ot Net ZRuntime::Net has a single worker thread and is also where transport teardown runs (handle_close in the transport RX loop), which calls TaskController::terminate_all -- a blocking call via block_in_place that stalls Net's one worker for up to its own timeout. Queuing the send_interest replay task on Net too serialized it behind every concurrent face teardown, causing three_node_combination to hang under CI load with many sessions closing at once. Application's multi-worker pool avoids that bottleneck.
fuzzypixelz
reviewed
Jul 23, 2026
Member
There was a problem hiding this comment.
I was able to reproduce this way:
#/usr/bin/env nuze -X
let too_many_tokens = 1..257 | each { |x| z liveliness declare-token $"too-many-tokens/($x)" }#/usr/bin/env nuze -X
z liveliness get too-many-tokens/*
# deadlocks!
z liveliness sub --history too-many-tokens/*
# also deadlocks!I am against exceptionally making Face::send_interest async for non-FINAL mode. It it an ad-hoc escape hatch in an otherwise consistent sync design within the routing layer.
Let's not forget that the root cause of our troubles here is that (1) Zenoh callbacks can run arbitrary blocking code inside ZRuntime and that (2) the stable API itself shoots itself in the foot by implementing blocking handlers such as FifoChannel.
If you had to take this course—which I don't agree with—I would suggest adding a flag to switch off the async behavior for the sake of keeping routing unit tests fast and deterministic, as they were explicitly designed to be. I've implemented disable_async_tree_computation for precisely this reason:
zenoh/zenoh/src/net/routing/gateway.rs
Lines 51 to 60 in 8358cd5
If any action is to be taken at all, one idea would be to change the DefaultHandler implementation to log an error on timeout:
@@
impl<T: Send + 'static> IntoHandler<T> for TimeoutFifoChannel {
type Handler = TimeoutFifoChannelHandler<T>;
fn into_handler(self) -> (Callback<T>, Self::Handler) {
let (sender, receiver) = flume::bounded(self.capacity);
(
Callback::from(move |t| {
- if let Err(error) = sender.send(t) {
+ if let Err(error) = sender.send_timeout(t, FIFO_CHANNEL_SEND_TIMEOUT) {
tracing::error!(%error)
}
}),
TimeoutFifoChannelHandler(receiver),
)
}
}This way, the system would drop the token and log an error instead of dead-locking. I don't know which one is worse!
Another idea would be to change the DefaultHandler for liveliness queries to an unbounded FIFO queue. Gateways send all tokens downstream on every liveliness query, so we don't exactly store all tokens received through liveliness queries, but we've considered it in the past, and opted out for ease of implementation instead of concerns about memory usage. Thus is idea of temporarily allocating all known tokens in memory does not frighten me as a default.
Let's do a back-of-the-envelope calculation of allocated memory for 10k tokens. Sample is 224 bytes on my host with shared-memory enabled. For a key-expression of length 32 we get a 2.5MB spike in memory usage.
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
Session::liveliness_query(and any query whose replies go through the default bounded FIFO handler) can self-deadlock the calling thread.Face::send_interestwalks the routing table and replays every currently-live matching token onto the query's reply channel synchronously, on the same thread that is about to start draining that channel. Once the number of matching results exceeds the handler's default capacity (256 slots), the synchronous send blocks forever waiting for a consumer that structurally cannot run until the function that is blocked returns.This was originally diagnosed against a downstream ROS 2/zenoh integration hitting it in production on late-joiner nodes with many active peer sessions, and worked around there with a call-site fix (a larger reply-handler capacity) — see ZettaScaleLabs/hiroz#226. This PR fixes the underlying zenoh-core issue directly.
The fix moves the post-lock replay loop off the calling thread, so the caller can return and start draining its reply channel concurrently with the replay instead of blocking on it.
Key Changes
zenoh/src/net/routing/dispatcher/face.rs:Face::send_interest's post-locksend_declarereplay loop now runs viaself.state.task_controller.spawn_with_rt(zenoh_runtime::ZRuntime::Net, ...)instead of inline on the calling thread. The table walk underctrl_lockis unchanged — it must stay synchronous for table consistency; only the replay fan-out moves. The actualsend_declarecalls run insideZRuntime::Net.spawn_blocking(...), since they can invoke arbitrary user callbacks that block synchronously for an unbounded time; running that inline on an async worker would starve other work sharing that runtime, includingSession::close_inner's own teardown. A panic during the replay is logged (tracing::error!) rather than silently discarded.zenoh/tests/liveliness_self_deadlock_{peer,router}.rs: new regression tests, each self-contained in its own process. Each declares 300 liveliness tokens and issues a liveliness query against its own local table using the default 256-slot handler — the exact vulnerable path. Asserts both prompt completion and that all 300 replies are delivered.zenoh/src/net/tests/regions/mod.rs: the mock routing-test harness callsFace::send_interestdirectly and asserts on it synchronously; give the now-async replay a brief moment to land before returning.zenoh/tests/routing.rs:three_node_combination's liveliness recipes now exercise the replay's new async hop throughZRuntime::Net, which defaults to a single worker thread shared across the whole process (includingSession::close_inner's own teardown). Under CI's contended runners that added scheduling latency occasionally exceeded the file's shared 10sTIMEOUT. Rather than loosening that constant for every test in the file,Recipe::rungained arun_with_timeoutvariant, and onlythree_node_combination's recipe calls now use a dedicated, widerTHREE_NODE_COMBINATION_TIMEOUT(90s); all other tests (gossip*,static_failover_brokering,two_node_combination,three_node_combination_multicast,router_linkstate) keep the original 10s budget.Verification
Direct before/after confirmation: on the parent commit (plain synchronous replay),
liveliness_self_deadlock_peerhangs and is killed by its wrapper timeout. On this PR's commits, it andliveliness_self_deadlock_routerboth pass in ~0.5s with 300/300 replies drained. Reply ordering is preserved — the collected token declares and theDeclareFinalcompletion marker travel through the same replay task in original order.🏷️ Label-Based Checklist
Based on the labels applied to this PR, please complete these additional requirements:
Labels:
bug,api fix🐛 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.