Fix the full-node teardown hang: cancel and suspend the joinScope drain#92
Conversation
WebrtcFullNodeNetworkTest.HappyPath hit the 1200 s ctest timeout twice on the ubuntu-latest leg of PR #90: after one concurrent start threw "Timed out" (waitForNetworkConnectivity, 10 s), the process went silent for ~20 minutes until ctest killed it. Runtime-evidence diagnosis (logs + a worker-pool-saturation reproduction) confirmed the mechanism: - Every node's start() had already detached a fire-and-forget joinDht into NetworkStack's GuardedAsyncScope. TearDown drives all nine stop() coroutines on ONE blockingWait thread; the first stop()'s joinScope.close() blocked that thread AND waited for its join to complete NATURALLY (nothing cancelled it first, violating the scope's own cancel-before-close contract - the DhtNode abort only happens later in the stop sequence and cannot be reordered past the ContentDeliveryManager teardown-ordering constraints). - A join only completes with shared-worker-pool capacity and network progress: every coroutine resumption, including folly::coro::timeout resumptions, needs a pool slot. The 8-node start storm starved the 4-thread CI pool, so the drain never returned. Locally, saturating SharedExecutors::worker() reproduced the identical silent hang; the queued joinDht started the same millisecond a pool thread freed. The fix removes teardown's dependence on join completion: - NetworkStack: a CancellationSource wraps the detached joinDht; stop() requests cancellation BEFORE draining, and drains via the new suspending GuardedAsyncScope::closeAsync() so concurrent sibling stops sharing the drive thread proceed meanwhile. - DiscoverySession/RingDiscoverySession: MERGE the ambient cancellation token with the node's abort signal instead of replacing it - co_withCancellation replaces the ambient token, which made stop()-time cancellation invisible inside the sessions' RPC awaits. - PeerDiscovery::runSessions: a join cancelled by its owner's stop() no longer schedules a detached rejoin. - CoroutineHelper: export co_current_cancellation_token and cancellation_token_merge shims (CPOs, same pattern as blockingWait). Verified with the instrumented reproduction: post-fix, all nine joinScope drains enter in the same millisecond, cancelled joins collapse in ~1.5 s, and the failed-SetUp teardown takes 2.5 s (was: wedged for the whole pool-starvation window, 19+ minutes on CI). Under a 20 s artificial pool saturation, teardown completes ~5 s after the first pool liveness instead of never. GuardedAsyncScopeTest adds the deadlock-shape regression test; the dht end-to-end join suite (7 tests) passes under heavy load with the cancellation plumbing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…troy() Same mechanism and fix as NetworkStack's joinScope in the parent commit, one layer down: destroy()'s blocking joinScope.close() drained un-cancelled startLayersAndJoinDht/handleEntryPointLeave tasks on the shared blockingWait drive thread — unbounded under a starved worker pool (sampled as ContentDeliveryManager.cppm:209 -> GuardedAsyncScope::close() wedging the WebrtcFullNodeNetworkTest teardown on a CI-sized 4-thread pool). A joinCancellation source now wraps all four detached joinScope tasks; destroy() requests cancellation first and drains via the suspending closeAsync(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fire-and-forget replication coroutines were started with a bare .start(): they pin the StoreManager via sharedFromThis, but their RPC remotes send through the owning DhtNode's communicator, which nothing kept alive — a replication resuming after DhtNode::stop() dereferenced the freed communicator (SIGSEGV in RpcCommunicatorClientApi::notify<ReplicateDataRequest> resume, reproduced 13/16 on a CI-sized 4-thread pool once the teardown drain no longer serialized everything). Both launch sites now go through a cancellable GuardedAsyncScope that destroy() cancels and drains while the communicator and transport are still alive; the awaited final leave-replication is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both wrapped their RPC round-trip in a thread-blocking blockingWait(). On the shared worker pool that is a self-deadlock: once poolsize concurrent joins sit inside lockConnection at the same time, every pool thread waits for a lock-RPC response that only those same threads could process (sampled: all four StreamrWorkers of a CI-sized pool blocked in PeerDiscovery::joinThroughEntryPoint -> lockConnection -> blockingWait, test wedged). The interface and the ConnectionManager implementation now return Task<void>; PeerDiscovery co_awaits them. ProxyClient's synchronous connection routine wraps the calls in blockingWait at its own call sites for now — it already blocks on its other RPCs, and its callers block at the C-API boundary; async-ifying that path is a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The server-dispatch scope only drained in ~RoutingRpcCommunicator during member destruction — after later-declared members were already gone — so a straggler getClosestPeers dispatch resumed into freed memory (crash reports on the CI-sized pool; the teardown-drain-ordering rule). stop() now removes the transport listeners with offAndWait() (a Message handler mid-invocation can otherwise schedule one more dispatch past the drain) and calls rpcCommunicator->drainAsyncTasks() as its last step, after the leave notices and final replications that still send through the communicator. The RPC client/server APIs gate their scope-add paths on the drained flag: folly forbids add-after-join, so a late request now fails cleanly (client) or is dropped like any request to a stopped node (server). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both destroy() methods held the connector mutex across the pendingConnection->close(true) call-outs. The Disconnected fan-out re-enters connection and connector code, so this deadlocks ABBA against any thread that holds a connection's mutex and needs the connector's in its handler chain (sampled: main thread in WebsocketClientConnector::destroy -> PendingConnection::close -> Disconnected handler blocked on a connection mutex, an rtc close callback holding it and blocked on the connector mutex, a pool worker in connect() queued behind — the never-hold-locks-across-call-outs rule). Both now snapshot and clear their maps under the lock and make every call-out after releasing it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous commits were formatted with the system clang-format; this re-runs run-clang-format.py -i with the same binary lint.sh checks against, mostly reverting cosmetic reflow the wrong version introduced. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e2362a8 to
d79c601
Compare
Completed the teardown-hang fix: four more defects diagnosed and fixed on this branchThe original joinScope fix was necessary but not sufficient — this PR's own CI run still timed out on both full-node tests (ubuntu + arm64) and segfaulted on macOS. Reproducing under a CI-emulated worker pool (
Validation (CI-emulated 4-thread pool, The one residual failure (1/16 under pathological load) is a distinct pre-existing UAF ( 🤖 Generated with Claude Code |
The restored incremental-build cache on the macos-26 leg carried a pre-#92 BMI of this module and under-rebuilt: DiscoverySession and RingDiscoverySession failed to compile against the cancellation shims this branch added, while identical code built and passed on both Linux legs and locally. Touching the module interface forces the BMI (and its dependents) to rebuild — the documented remedy for this cache trap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
WebrtcFullNodeNetworkTest.HappyPathhit the 1200 s ctest timeout twice in a row on the ubuntu-latest leg of #90 (run 29248801288, attempt 1): one of the eight concurrentNetworkStack::start()calls threw folly'sFutureTimeout("Timed out") fromwaitForNetworkConnectivityat exactly +10 s, and the process then produced no output for ~20 minutes until ctest killed it. The same commit passed on the macos-26 and linux-arm64 legs.Root cause (runtime-evidence diagnosis)
By the time the exception fires, every node has already detached a fire-and-forget
joinDhttask intoNetworkStack'sGuardedAsyncScope. In TearDown, all ninestop()coroutines run on oneblockingWaitdrive thread, and the firststop():joinScope.close(), freezing every sibling stop behind it.The drain therefore waits for the join to complete naturally, which needs shared-worker-pool capacity — every coroutine resumption, including
folly::coro::timeoutresumptions, needs a pool slot. The 8-node start storm starved the 4-thread CI pool (the CI log shows zero websocket accepts for the whole 10 s window), so the drain never returned. SaturatingSharedExecutors::worker()locally reproduced the identical silent hang, and the queuedjoinDhtstarted the same millisecond a pool thread freed — confirming the dependency.Fix
CancellationSourcewraps the detachedjoinDht;stop()requests cancellation before draining, and drains via the new suspendingGuardedAsyncScope::closeAsync()so concurrent sibling stops proceed meanwhile.closeAsync()(suspends instead of blocking; syncclose()kept for destructors).co_withCancellationreplaces the ambient token, which made stop()-time cancellation invisible inside the sessions' RPC awaits.co_current_cancellation_token/cancellation_token_mergeshims (CPOs, same pattern as the existingblockingWaitshim).Verification
GuardedAsyncScopeTestadds the deadlock-shape regression test (drain that only completes if a sibling stop can progress — hangs with blockingclose()).Distinct follow-up (pre-existing, not addressed here): the entry point accepting zero websocket connections for the whole 10 s window is the known accept-path degradation that made SetUp fail in the first place.
🤖 Generated with Claude Code