Skip to content

fix: make the gossip autoconnect task abortable - #2733

Open
ryandavid wants to merge 1 commit into
eclipse-zenoh:mainfrom
ryandavid:fix/abortable-gossip-autoconnect
Open

fix: make the gossip autoconnect task abortable#2733
ryandavid wants to merge 1 commit into
eclipse-zenoh:mainfrom
ryandavid:fix/abortable-gossip-autoconnect

Conversation

@ryandavid

@ryandavid ryandavid commented Aug 8, 2026

Copy link
Copy Markdown

Description

Session close could hang forever under peer churn. Runtime::close_inner waits on terminate_all_async(), which waits for every tracked task with no timeout, and gossip's autoconnect was spawned with spawn -- so the cancellation token never reached it, and it sat in connect_peer() against a peer that was itself shutting down.

That does not meet spawn's documented contract, which requires the task to be cancellable or to finish in finite time; connecting to a remote peer is neither. spawn_abortable is what the multicast scouting side already uses for the same work (autoconnect_all, orchestrator.rs:343 and :360). The task stays tracked, so shutdown still waits for it, but it now observes the token and finishes.

token.cancel() is only reached from terminate_all and terminate_all_async, so nothing changes during normal operation.

Adds a regression test, which fails without this change with "close operation timed out" and passes with it.

What does this PR do?

Spawns gossip's autoconnect task with spawn_abortable instead of spawn, so
that session close can no longer wait on it for ever.

-                    strong_runtime.spawn(async move {
+                    strong_runtime.spawn_abortable(async move {

It also adds a regression test, zenoh/tests/close_under_peer_churn.rs.

Why is this change needed?

Two peer sessions that discover each other over multicast and then open and close repeatedly deadlock forever inside session close. One process alone always finishes. Two, started together, 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, not by reading them. The marker before terminate_all_async prints; the one after it never does, so manager.close() is never reached and the transports are never closed:

// zenoh/src/net/runtime/mod.rs
async fn close_inner(&self, _: ()) {
    self.task_controller.terminate_all_async().await;   // <-- never returns
    self.manager.close().await;                         // <-- never reached
    ...
}

terminate_all_async() is unbounded - it waits for every tracked task, with no timeout:

// commons/zenoh-task/src/lib.rs
pub async fn terminate_all_async(&self) {
    self.tracker.close();
    self.token.cancel();
    self.tracker.wait().await
}

and the task it waits for is gossip's autoconnect, spawned with spawn:

// zenoh/src/net/protocol/gossip.rs:390
strong_runtime.spawn(async move {
    if runtime.manager().get_transport_unicast(&zid).await.is_none() {
        runtime.start_conditions().add_peer_connector_zid(zid).await;
        if runtime.connect_peer(&zid, &locators).await && ... {
            runtime.start_conditions().terminate_peer_connector_zid(zid).await;
        }
    }
});

token.cancel() therefore does nothing to it, and it is sitting in connect_peer() against a peer that is itself shutting down.

spawn's own documentation says it is for a task that

can be cancelled [by] a token obtained by TaskController::get_cancellation_token(), was created via TaskController::into_abortable(), or can run to completion in finite amount of time

and terminate_all puts that obligation explicitly on the caller:

The caller must ensure that all tasks spawned with TaskController::spawn()… can yield in finite amount of time

Connecting to a remote peer is not finite, and this task holds no cancellation token, so neither branch of the contract holds. The equivalent task on the multicast scouting path - the same job, connecting to a peer that discovery just turned up - already uses spawn_abortable: autoconnect_all (orchestrator.rs:343, :360) and the scouting responder (orchestrator.rs:223, :343, :355). Gossip looks like the odd one out.

spawn_abortable still tracks the task, so terminate_all_async still waits for it; the task simply now observes the token and finishes instead of never. token.cancel() is only ever reached from terminate_all and terminate_all_async, so this changes nothing during normal operation - aborts happen only at shutdown.

How to reproduce

A ~20-line program, default config, nothing else:

#include <zenoh.hxx>
#include <cstdio>
#include <unistd.h>

int main()
{
    for (int cycle = 0; cycle < 50; ++cycle)
    {
        std::fprintf(stderr, "[%d] cycle %d\n", getpid(), cycle);
        zenoh::Session::open(zenoh::Config::create_default());
    }
    std::fprintf(stderr, "[%d] done -- did not deadlock\n", getpid());
    return 0;
}
./repro & ./repro & wait

One process alone always completes all 50 cycles. Two hang, typically by cycle 3-12; the last line each prints is the cycle it died on, and it never recovers (observed >10 minutes). While hung, every zenoh runtime worker is idle in kevent with nothing runnable, so this is a wakeup that never arrives rather than contention with a running thread.

Related Issues

Related to #2409 and tokio-rs/tokio#7892 (the block_in_place family).

Not fixed by #2637, which was the obvious candidate - same file, same family (block_in_place parking the ctrl_lock holder on a tokio mutex under peer churn, which is exactly this workload). It was applied in full and measured first: still hung 6 runs out of 8. Its two hunks are in terminate_peer_connector_zid's callers; this is a different task in the same function that is never cancelled. Both look worth having; only this one closes this hang.


🏷️ 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:

  • Root cause documented - Explain what caused the bug in the PR description
  • Reproduction test added - Test that fails on main branch without the fix
  • Test passes with fix - The reproduction test passes with your changes
  • Regression prevention - Test will catch if this bug reoccurs in the future
  • Fix is minimal - Changes are focused only on fixing the bug
  • Related bugs checked - Verified no similar bugs exist in related code

Why this matters: Bugs without tests often reoccur.

Instructions:

  1. Check off items as you complete them (change - [ ] to - [x])
  2. The PR checklist CI will verify these are completed

This checklist updates automatically when labels change, but preserves your checked boxes.

Session close could hang forever under peer churn. `Runtime::close_inner` waits
on `terminate_all_async()`, which waits for every tracked task with no timeout,
and gossip's autoconnect was spawned with `spawn` -- so the cancellation token
never reached it, and it sat in `connect_peer()` against a peer that was itself
shutting down.

That does not meet `spawn`'s documented contract, which requires the task to be
cancellable or to finish in finite time; connecting to a remote peer is neither.
`spawn_abortable` is what the multicast scouting side already uses for the same
work (`autoconnect_all`, orchestrator.rs:343 and :360). The task stays tracked,
so shutdown still waits for it, but it now observes the token and finishes.

`token.cancel()` is only reached from `terminate_all` and
`terminate_all_async`, so nothing changes during normal operation.

Adds a regression test, which fails without this change with "close operation
timed out" and passes with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ryandavid added a commit to redline-labs/digital_dashboard that referenced this pull request Aug 21, 2026
The close-deadlock fix is still not upstream -- eclipse-zenoh/zenoh#2733 is
open and gossip.rs is unchanged on release/1.10.0 and on main -- so the patch
stays. It applies to 1.10.0 without a rebase; only the pinned revision moves,
to the one zenoh-c 1.10.0's lockfile resolves.

Worth the bump for two fixes that land on this topology: multicast scouting
now works on loopback (macOS refused Scout from a loopback-bound socket, and
Hello omitted loopback locators, which broke loopback-only hosts everywhere),
and the runtime reconnects on transport error paths it used to miss.

Also drop the ZENOHCXX_BUILD_* spellings, which 1.10.0 no longer declares at
all, and correct the dependency table -- zenoh-cpp was listed at 1.4.0 and
zenoh-c, the patched one, was missing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GQCyThaFqnYekXQyPUufMm
@ixil

ixil commented Aug 21, 2026

Copy link
Copy Markdown

The cause is actually a regression of PR #2012 due to PR #2173

  • 252f6a8 — PR Fix Session::close timeout while client attempts to reconnect #2012, "Fix Session::close timeout while client attempts to reconnect", 2025-07-24. Changed session.runtime.spawn(...) → spawn_abortable(...) in closed_session. Fixed exactly this, for exactly this reason.
  • 2250856 — PR Fix multlink reconnect #2173, "Fix multlink reconnect", 2025-10-09. Rewrote closed_session for an unrelated purpose. The WhatAmI::Client arm carrying spawn_abortable was deleted, and the replacement uses two plain spawn calls. The close-timeout fix was silently lost and the site count doubled.

Issue #2675 is also due to this.

We hit this independently and arrived at the same diagnosis, so first: this analysis is correct, and spawn_abortable is the right remedy rather than a hand-written select!run_until_cancelled_owned drops the task at its next await point, which is what the connect needs.

The fix looks incomplete, though: there is a second call site with the same defect, and this PR does not cover it.
zenoh/src/net/runtime/orchestrator.rs:876 spawns the peer connector with plain spawn:

self.spawn(async move {
    if let Ok(zid) = this.peer_connector_retry(peer).await {

That is the configured connect.endpoints path, whereas gossip.rs:390 is the gossip-discovery path. Same uncancellable open_transport_unicast await underneath, reached two different ways. With only gossip.rs fixed, a deployment that lists peers in connect.endpoints and does not rely on gossip
still hangs on close.

Reproducing it without gossip at all

The gossip route needs two peers and multicast churn. The configured route needs neither, which makes for a smaller repro:

  • one session, scouting.multicast.enabled: false, scouting.gossip.enabled: false`
  • a single connect.endpoints entry pointing at a plain TcpListener that accepts the connection and then sends nothing
  • open, then close
    Close time tracks transport.unicast.open_timeout directly:
    | open_timeout | open | close |
    | -------------- | --------- | --------- |
    | 10000 ms | 509.95 ms | 9.50 s |
    | 2000 ms | 504.61 ms | 1.50 s |

The trigger condition is narrower than "unreachable"

A refused connection returns immediately and the retry sleep that follows is already cancellable via wait_next_peer_retry — no hang. The hang needs an endpoint that completes the TCP handshake but not the zenoh handshake: a paused or frozen container (podman pause reproduces it in one command), a firewall that DROPs, or a wedged process. The distinction is important.

This is also why we think #2675 ("session.close() blocks ~10s ... when the peer link has active bursty packet loss") is the same root cause reported from the user side — packet loss on an established link stalls the same await.

Severity bound

The stall does not stack across endpoints — three configured endpoints still cost one open_timeout, not three, because the connector tasks overlap. Worth stating so the bug is not read as worse than it is.

One more site worth a look, not asserted

orchestrator.rs:578 (spawn_add_listeneradd_listener_retry) is also a plain spawn wrapping a retry loop. We have not shown it can hang and are not claiming it does — but it is the same shape, and if the two above are being changed it may be worth auditing at the same time.

AI disclaimer - much of this was found when I was doing work with an agent, and I had it investigate and write things up

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.

3 participants