From 4f4c6b7a164139e9af7af1b2e6f7db09286b821c Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 19 Oct 2026 18:41:21 -0500 Subject: [PATCH] fix(rpc): drop a stale connect result after teardown; don't let open() erase a pending abort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Concurrency audit, connect-lifecycle races (no fund-path call touched — connect-result gating + abort-flag lifetime only): - The connect task's MainCb ran onConnected() on any successful probe without re-checking that a wallet switch / adopt / restore / shutdown had already torn the connection down — so a probe that succeeded against the old daemon could start a refresh against one about to be stopped. Capture rpc_session_ when posting (main thread) and, in the MainCb, drop the result (requestAbort + disconnect, not a blocking bare disconnect) when the session changed or daemon_restarting_/shutting_down_ is set. Same guard on the cookie-auth retry post. - RPCClient::open() called clearAbort(), so a requestAbort() issued while a connect was still queued was erased, stalling teardown/shutdown. open() no longer clears it; the two reconnect entry points (tryConnect for rpc_, onConnected for fast_rpc_) clear it on the main thread first, skipped while shutting_down_ so a shutdown abort survives. A fresh client defaults to not-aborted, so first connects are unaffected. HELD (not in this commit): making abortMaintenanceCalls() abort unconditionally (the UI-freeze-on-disconnect finding) — it would also abort an in-flight user send during teardown, a send-lifecycle behaviour change that wants explicit sign-off (it is fund-safe via the lost-reply "outcome unknown" handling, but still a behaviour change). Threading fix — clean build + ctest; the races are runtime-only. Co-Authored-By: Claude Opus 4.8 --- src/app_network.cpp | 34 +++++++++++++++++++++++++++++----- src/rpc/rpc_client.cpp | 3 ++- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/app_network.cpp b/src/app_network.cpp index 6dc4f61..93843af 100644 --- a/src/app_network.cpp +++ b/src/app_network.cpp @@ -563,15 +563,31 @@ void App::tryConnect() attempt, isUsingEmbeddedDaemon() ? "yes" : "no"); } - worker_->post([this, config, daemonStarting, externalDetected, attempt]() -> rpc::RPCWorker::MainCb { + // A previous teardown leaves rpc_'s abort flag set (open() no longer clears it, so an abort issued + // while this connect is still queued is not erased). This is the reconnect entry point: clear it here + // on the main thread, after the daemon_restarting_/shutting_down_ gates above. + if (rpc_ && !shutting_down_.load()) rpc_->clearAbort(); + const std::uint64_t connectSession = rpc_session_; // main thread; a teardown/onConnected bumps it + worker_->post([this, config, daemonStarting, externalDetected, attempt, connectSession]() -> rpc::RPCWorker::MainCb { bool connected = rpc_->connect(config.host, config.port, config.rpcuser, config.rpcpassword, config.use_tls); std::string connectErr = rpc_->getLastConnectError(); const rpc::RpcFailureKind connectKind = rpc_->getLastConnectFailureKind(); bool warmingUp = rpc_->isWarmingUp(); std::string warmupStatus = rpc_->getWarmupStatus(); - + return [this, config, connected, warmingUp, warmupStatus, daemonStarting, externalDetected, attempt, connectErr, - connectKind]() { + connectKind, connectSession]() { + // A probe that succeeded after a wallet switch / adopt / restore / shutdown already tore the + // connection down must not run onConnected() against a daemon that is about to be stopped. + if (connected && (connectSession != rpc_session_ || daemon_restarting_.load() || shutting_down_.load())) { + VERBOSE_LOGF("[connect #%d] Dropping stale connect result (teardown raced the probe)\n", attempt); + if (rpc_) { + rpc_->requestAbort(); // a worker call may hold curl_mutex_; don't block the UI on disconnect() + rpc_->disconnect(); + } + connection_in_progress_ = false; + return; + } noteDaemonRpcResult(connected, connectKind); // a busy node (timeouts / queue full) feeds the quit guard if (connected) connect_backoff_.reset(); if (connected) { @@ -600,12 +616,19 @@ void App::tryConnect() if (rpc::Connection::buildCookieAuthConfig(config, cookieConfig)) { VERBOSE_LOGF("[connect #%d] HTTP 401 — retrying with .cookie auth from %s\n", attempt, cookieConfig.hush_dir.c_str()); - worker_->post([this, cookieConfig, attempt]() -> rpc::RPCWorker::MainCb { + worker_->post([this, cookieConfig, attempt, connectSession]() -> rpc::RPCWorker::MainCb { bool ok = rpc_->connect(cookieConfig.host, cookieConfig.port, cookieConfig.rpcuser, cookieConfig.rpcpassword, cookieConfig.use_tls); - return [this, cookieConfig, ok, attempt]() { + return [this, cookieConfig, ok, attempt, connectSession]() { connection_in_progress_ = false; + if (ok && (connectSession != rpc_session_ || daemon_restarting_.load() || shutting_down_.load())) { + if (rpc_) { + rpc_->requestAbort(); + rpc_->disconnect(); + } + return; // stale: a teardown raced the probe + } if (ok) { VERBOSE_LOGF("[connect #%d] Connected via .cookie auth\n", attempt); saved_config_ = cookieConfig; @@ -836,6 +859,7 @@ void App::onConnected() fast_worker_->start(); } // Connect on the fast worker's own thread (non-blocking to main) + if (fast_rpc_ && !shutting_down_.load()) fast_rpc_->clearAbort(); // open() no longer clears a prior teardown's abort fast_worker_->post([this]() -> rpc::RPCWorker::MainCb { bool ok = fast_rpc_->connect(saved_config_.host, saved_config_.port, saved_config_.rpcuser, saved_config_.rpcpassword, diff --git a/src/rpc/rpc_client.cpp b/src/rpc/rpc_client.cpp index 87805e8..a703115 100644 --- a/src/rpc/rpc_client.cpp +++ b/src/rpc/rpc_client.cpp @@ -216,7 +216,8 @@ bool RPCClient::open(const std::string& host, const std::string& port, curl_easy_setopt(impl_->curl, CURLOPT_HTTPHEADER, impl_->headers); curl_easy_setopt(impl_->curl, CURLOPT_WRITEFUNCTION, WriteCallback); // Progress callback so requestAbort() can unblock an in-flight curl_easy_perform. - clearAbort(); // a fresh connection must not start in the aborted state + // NOTE: abort is deliberately NOT cleared here: a requestAbort() issued while this connect was queued + // must still cut it short. Reconnect callers (App::tryConnect / onConnected) call clearAbort() first. curl_easy_setopt(impl_->curl, CURLOPT_NOPROGRESS, 0L); curl_easy_setopt(impl_->curl, CURLOPT_XFERINFOFUNCTION, xferInfoCallback); curl_easy_setopt(impl_->curl, CURLOPT_XFERINFODATA, this);