From 38ee719ac54e95b636e6a7b3c87c64564074b363 Mon Sep 17 00:00:00 2001 From: KSEGIT Date: Thu, 9 Jul 2026 15:21:35 +0100 Subject: [PATCH 1/2] add regression test for bounded session teardown with LSD active With LSD enabled and an announcing torrent, abort() + session_proxy destruction must complete within a bounded time. Guards against the shutdown wedges reported in qbittorrent/qBittorrent#24353 (fixed by 4138b27d5, 38d78f022 and the following commit). A watchdog (joined via a scope guard, so an exception cannot unwind through a joinable thread) converts any future teardown deadlock into a fast, attributed failure instead of an indefinite test hang. Registered as explicit, like test_lsd, and added to the Makefile dist list. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JbRhL98sdGs31KAXE9uaj7 --- .github/workflows/linux.yml | 2 +- Makefile | 1 + test/Jamfile | 2 + test/test_lsd_teardown.cpp | 125 ++++++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 1 deletion(-) create mode 100644 test/test_lsd_teardown.cpp diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml index c7e41bf65dc..bd0f63c053a 100644 --- a/.github/workflows/linux.yml +++ b/.github/workflows/linux.yml @@ -303,7 +303,7 @@ jobs: run: | cd test b2 ${{ matrix.config }} -l500 warnings-as-errors=on debug-iterators=on asserts=on deterministic-tests - b2 ${{ matrix.config }} -l500 testing.execute=off warnings-as-errors=on debug-iterators=on asserts=on test_lsd test_hasher test_hasher512 test_natpmp enum_if + b2 ${{ matrix.config }} -l500 testing.execute=off warnings-as-errors=on debug-iterators=on asserts=on test_lsd test_lsd_teardown test_hasher test_hasher512 test_natpmp enum_if b2 ${{ matrix.config }} -l500 warnings-as-errors=on debug-iterators=on asserts=on simulate-slow=hash test_slow_hash b2 ${{ matrix.config }} -l500 warnings-as-errors=on debug-iterators=on asserts=on simulate-slow=write test_disk_io diff --git a/Makefile b/Makefile index a942adc4b78..be32f9750d0 100644 --- a/Makefile +++ b/Makefile @@ -951,6 +951,7 @@ TEST_SOURCES = \ test_ip_voter.cpp \ test_listen_socket.cpp \ test_lsd.cpp \ + test_lsd_teardown.cpp \ test_magnet.cpp \ test_merkle.cpp \ test_merkle_tree.cpp \ diff --git a/test/Jamfile b/test/Jamfile index 17271556848..ce6d135f186 100644 --- a/test/Jamfile +++ b/test/Jamfile @@ -251,6 +251,8 @@ run test_priority.cpp ; run test_upnp.cpp ; run test_lsd.cpp ; explicit test_lsd ; +run test_lsd_teardown.cpp ; +explicit test_lsd_teardown ; run test_hasher.cpp ; explicit test_hasher ; run test_hasher512.cpp ; diff --git a/test/test_lsd_teardown.cpp b/test/test_lsd_teardown.cpp new file mode 100644 index 00000000000..44c4c101f30 --- /dev/null +++ b/test/test_lsd_teardown.cpp @@ -0,0 +1,125 @@ +/* + +Copyright (c) 2026, KSEGIT +All rights reserved. + +You may use, distribute and modify this code under the terms of the BSD license, +see LICENSE file. +*/ + +#include "libtorrent/session.hpp" +#include "libtorrent/settings_pack.hpp" +#include "libtorrent/add_torrent_params.hpp" +#include "libtorrent/magnet_uri.hpp" +#include "libtorrent/error_code.hpp" + +#include "libtorrent/aux_/scope_end.hpp" + +#include "test.hpp" +#include "test_utils.hpp" // for test_listen_interface() + +#include +#include +#include +#include +#include +#include + +// Regression guard for the macOS shutdown freeze (qbittorrent/qBittorrent#24353) +// and the libtorrent session-destructor hang (#4510). +// +// With Local Service Discovery enabled and an active torrent, the single +// io_context thread periodically runs on_lsd_announce -> announce_lsd -> +// lsd::announce_impl. Historically that performed a *synchronous*, uncancellable +// multicast send_to(). When the outbound interface stalls (e.g. a VPN tunnel +// that vanishes across sleep/wake) the send blocks in-kernel forever, the io +// thread never returns to its run loop, abort() (dispatched onto that same +// thread) never runs, and session_proxy::~session_proxy()'s std::thread::join() +// deadlocks. Downstream this is the macOS "must force quit" freeze. +// +// This test pins the portable invariant the fix protects: with LSD active, +// aborting the session and destroying the session_proxy completes within a +// bounded time. The bound is deliberately generous (30s vs the few +// milliseconds a healthy teardown takes) so the test is not timing-flaky; a +// true deadlock blows well past it. Note: on a healthy CI interface a +// blocking send also returns quickly, so this test cannot simulate the +// stalled-interface trigger itself (nor does it exercise the uTP teardown +// path - no peer connections exist). On hosts where no interface can join +// the LSD multicast group the announce is skipped entirely and only the +// bounded-teardown invariant is exercised. The test guards that invariant +// and, via the watchdog below, makes any future teardown deadlock fail fast +// with attribution instead of hanging the test binary. +TORRENT_TEST(lsd_teardown_is_bounded) +{ + using namespace lt; + using namespace std::chrono; + + // if the deadlock this test guards against regresses, the session_proxy + // destructor below never returns and the TEST_CHECK at the end is never + // reached. This watchdog converts that indefinite hang into a bounded, + // clearly-attributed failure instead of an opaque CI job timeout. + std::mutex done_mutex; + std::condition_variable done_cv; + bool done = false; + std::thread watchdog([&] { + std::unique_lock l(done_mutex); + if (done_cv.wait_for(l, seconds(90), [&] { return done; })) return; + std::fprintf(stderr, "test_lsd_teardown: TIMEOUT - session teardown " + "appears deadlocked (io thread not joinable)\n"); + std::abort(); + }); + // join via scope guard: if anything below throws, unwinding through a + // joinable std::thread would call std::terminate + auto join_watchdog = aux::scope_end([&] { + { + std::lock_guard l(done_mutex); + done = true; + } + done_cv.notify_one(); + watchdog.join(); + }); + + steady_clock::time_point start; + { + // declared first so it is destroyed last (joins the io thread) + session_proxy proxy; + + settings_pack pack; + pack.set_int(settings_pack::alert_mask + , alert_category::error | alert_category::status); + pack.set_bool(settings_pack::enable_dht, false); + pack.set_bool(settings_pack::enable_lsd, true); + pack.set_bool(settings_pack::enable_upnp, false); + pack.set_bool(settings_pack::enable_natpmp, false); + pack.set_str(settings_pack::listen_interfaces, test_listen_interface()); + + lt::session ses(pack); + + // add an (active) torrent so the LSD announce path actually fires + error_code ec; + add_torrent_params atp = parse_magnet_uri( + "magnet:?xt=urn:btih:0123456789abcdef0123456789abcdef01234567", ec); + TEST_CHECK(!ec); + atp.save_path = "."; + atp.flags &= ~torrent_flags::paused; + torrent_handle th = ses.add_torrent(atp, ec); + TEST_CHECK(!ec); + + // deterministically drive the LSD announce path rather than waiting + // for the periodic announce timer + th.force_lsd_announce(); + // give the io thread time to process the announce + std::this_thread::sleep_for(milliseconds(500)); + + start = steady_clock::now(); + proxy = ses.abort(); + // end of scope: ses is destroyed, then proxy's dtor joins the io thread + } + auto const elapsed = duration_cast(steady_clock::now() - start); + + std::printf("LSD session teardown took %d ms\n", int(elapsed.count())); + + // If the io thread had wedged in a synchronous LSD send, the join above + // would never return and we would never reach this line within the bound. + TEST_CHECK(elapsed < milliseconds(30000)); +} From dc64304e5b97b1206c5344b98590e0a993abde91 Mon Sep 17 00:00:00 2001 From: KSEGIT Date: Thu, 9 Jul 2026 15:21:35 +0100 Subject: [PATCH 2/2] abort uTP sockets when their UDP socket is torn down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit utp_stream stores its completion handlers in plain std::function members; each handler owns a shared_ptr to the peer_connection, and the utp_stream is itself a member of that peer_connection — a reference cycle (peer -> socket -> handler -> peer) that only utp_socket_impl::cancel_handlers() can break. session_impl::abort() closes the UDP sockets without telling the uTP socket managers: utp_socket_manager::remove_udp_socket() — the function designed for exactly this — had no callers. Worse, a uTP socket that had stalled on a send (EWOULDBLOCK, e.g. on an interface that went away across a sleep/wake cycle) loses its only wake-up when the UDP socket closes, because session_impl::on_udp_writeable drops the notification on error; and should_delete() requires !m_stalled, so the stalled socket can never be deleted. The result is a permanent shutdown hang: session_impl::on_tick() only initiates abort_stage2() once m_undead_peers is empty and m_utp_socket_manager.num_sockets() reaches zero. An orphaned uTP socket pins its peer_connection in m_undead_peers forever (and itself keeps num_sockets() > 0), so the tick re-arms every 100ms indefinitely, io_context::run() never returns, and session_proxy::~session_proxy()'s join() blocks forever. Observed live in qbittorrent/qBittorrent#24353: after a sleep/wake cycle with a VPN active, quitting qBittorrent hangs with the io thread ticking in kevent, zero open sockets, and the GUI blocked in the session teardown join. Fix: close each listen socket's UDP socket through a new close_udp_listen_socket() helper (used by both abort() and the runtime listen-socket removal path) which tells the uTP socket managers the socket is gone. remove_udp_socket() first flushes all stalled sockets via writable() — the same unfiltered delivery on_udp_writeable() performs — so sockets stalled on the closed socket fail their sends and tear down (clearing m_stalled, making them deletable), then aborts every uTP socket bound to the closed UDP socket. remove_udp_socket() requires the UDP socket to be closed first; this precondition is documented at the declaration and definition. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JbRhL98sdGs31KAXE9uaj7 --- ChangeLog | 1 + include/libtorrent/aux_/session_impl.hpp | 10 +++++++ .../libtorrent/aux_/utp_socket_manager.hpp | 3 ++ src/session_impl.cpp | 30 ++++++++++++++----- src/utp_socket_manager.cpp | 15 ++++++++++ 5 files changed, 52 insertions(+), 7 deletions(-) diff --git a/ChangeLog b/ChangeLog index 0dea91dfa10..3f3e8562a53 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,6 @@ 2.1.1 not released + * fix shutdown hang when uTP sockets are stalled on a closed UDP socket * require webtorrent RTC offer IDs to be exactly 20 bytes * fix point-to-point interfaces without a route to the internet being used for outgoing traffic * apply IP filter to DHT node diff --git a/include/libtorrent/aux_/session_impl.hpp b/include/libtorrent/aux_/session_impl.hpp index 5de95a222cc..ed780946742 100644 --- a/include/libtorrent/aux_/session_impl.hpp +++ b/include/libtorrent/aux_/session_impl.hpp @@ -768,6 +768,16 @@ namespace aux { void stop_ip_notifier(); void stop_lsd(); + + // closes the listen socket's UDP socket and aborts the uTP + // connections bound to it, so their completion handlers (holding + // peer_connection references) are released rather than leaked. + // As a side effect, every stalled uTP socket in the matching + // socket manager is woken, not just the ones bound to this + // socket; sockets stalled on other (still open) UDP sockets + // simply re-subscribe + void close_udp_listen_socket(std::shared_ptr const& s); + void stop_natpmp(); void stop_upnp(); diff --git a/include/libtorrent/aux_/utp_socket_manager.hpp b/include/libtorrent/aux_/utp_socket_manager.hpp index 743ad384be2..8c54a22b770 100644 --- a/include/libtorrent/aux_/utp_socket_manager.hpp +++ b/include/libtorrent/aux_/utp_socket_manager.hpp @@ -76,6 +76,9 @@ namespace aux { , error_code& ec, udp_send_flags_t flags = {}); void subscribe_writable(utp_socket_impl* s); + // aborts every uTP socket bound to this UDP socket, and wakes + // stalled sockets so they can be deleted. The UDP socket must have + // been closed before calling this void remove_udp_socket(std::weak_ptr sock); // internal, used by utp_stream diff --git a/src/session_impl.cpp b/src/session_impl.cpp index d6d5b2f3bbd..ffe7c8e2ecd 100644 --- a/src/session_impl.cpp +++ b/src/session_impl.cpp @@ -1109,12 +1109,12 @@ bool ssl_server_name_callback(ssl::stream_handle_type stream_handle, std::string TORRENT_ASSERT(!ec); } - // TODO: 3 closing the udp sockets here means that - // the uTP connections cannot be closed gracefully - if (l->udp_sock) - { - l->udp_sock->sock.close(); - } + // this closes the UDP socket abruptly; the uTP connections on it + // are aborted rather than closed gracefully. Leaving them alive + // would let their completion handlers pin peers in + // m_undead_peers (and keep num_sockets() > 0) forever, + // preventing on_tick() from ever completing the shutdown + close_udp_listen_socket(l); } // we need to give all the sockets an opportunity to actually have their handlers @@ -2204,7 +2204,7 @@ namespace { } #endif if ((*remove_iter)->sock) (*remove_iter)->sock->close(ec); - if ((*remove_iter)->udp_sock) (*remove_iter)->udp_sock->sock.close(); + close_udp_listen_socket(*remove_iter); if ((*remove_iter)->natpmp_mapper) (*remove_iter)->natpmp_mapper->close(); if ((*remove_iter)->upnp_mapper) (*remove_iter)->upnp_mapper->close(); if ((*remove_iter)->lsd) (*remove_iter)->lsd->close(); @@ -7184,6 +7184,22 @@ namespace { } } + void session_impl::close_udp_listen_socket(std::shared_ptr const& s) + { + if (!s->udp_sock) return; + s->udp_sock->sock.close(); + // the uTP socket manager requires the UDP socket to be closed + // before being told it is going away. Only the manager matching + // this socket's transport can have uTP sockets bound to it, so + // don't wake the other manager's stalled sockets spuriously +#ifdef TORRENT_SSL_PEERS + if (s->ssl == transport::ssl) + m_ssl_utp_socket_manager.remove_udp_socket(s); + else +#endif + m_utp_socket_manager.remove_udp_socket(s); + } + void session_impl::stop_natpmp() { for (auto& s : m_listen_sockets) diff --git a/src/utp_socket_manager.cpp b/src/utp_socket_manager.cpp index fa19c16f432..13ad035edae 100644 --- a/src/utp_socket_manager.cpp +++ b/src/utp_socket_manager.cpp @@ -266,9 +266,24 @@ namespace libtorrent::aux { m_drained_event.push_back(s); } + // precondition: the UDP socket must have been closed before this is + // called. Stalled sockets are woken below on the assumption that their + // send attempts fail against the closed socket, tearing the connection + // down; on a still-open (backpressured) socket they would just stall + // again void utp_socket_manager::remove_udp_socket(std::weak_ptr sock) { auto iface = sock.lock(); + + // sockets that stalled on this UDP socket will never receive their + // writable notification (the UDP socket is going away). Deliver it + // now: writable() clears the stalled state, which would otherwise + // prevent the socket from ever being deleted (should_delete() + // requires !m_stalled). Sockets stalled on other (still live) + // interfaces simply re-subscribe, just as they do when + // on_udp_writeable() flushes all stalled sockets + writable(); + for (auto& s : m_utp_sockets) { if (s.second->m_sock.lock() != iface)