Skip to content

Fix the full-node teardown hang: cancel and suspend the joinScope drain#92

Open
ptesavol wants to merge 7 commits into
mainfrom
claude/fix-full-node-teardown-hang
Open

Fix the full-node teardown hang: cancel and suspend the joinScope drain#92
ptesavol wants to merge 7 commits into
mainfrom
claude/fix-full-node-teardown-hang

Conversation

@ptesavol

Copy link
Copy Markdown
Collaborator

Problem

WebrtcFullNodeNetworkTest.HappyPath hit the 1200 s ctest timeout twice in a row on the ubuntu-latest leg of #90 (run 29248801288, attempt 1): one of the eight concurrent NetworkStack::start() calls threw folly's FutureTimeout ("Timed out") from waitForNetworkConnectivity at 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 joinDht task into NetworkStack's GuardedAsyncScope. In TearDown, all nine stop() coroutines run on one blockingWait drive thread, and the first stop():

  1. never cancels the detached join (violating the scope's documented cancel-before-close contract — the DhtNode abort happens later in the stop sequence and cannot be reordered past the ContentDeliveryManager teardown-ordering constraints), and
  2. drains it via the thread-blocking 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::timeout resumptions, 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. Saturating SharedExecutors::worker() locally reproduced the identical silent hang, and the queued joinDht started the same millisecond a pool thread freed — confirming the dependency.

Fix

  • NetworkStack: a CancellationSource wraps the detached joinDht; stop() requests cancellation before draining, and drains via the new suspending GuardedAsyncScope::closeAsync() so concurrent sibling stops proceed meanwhile.
  • GuardedAsyncScope: new closeAsync() (suspends instead of blocking; sync close() kept for destructors).
  • 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: a join cancelled by its owner's stop() no longer schedules a detached rejoin.
  • CoroutineHelper: co_current_cancellation_token / cancellation_token_merge shims (CPOs, same pattern as the existing blockingWait shim).

Verification

  • Instrumented before/after under the injected CI failure shape (one node's connectivity wait times out while siblings are mid-start): pre-fix the drains ran strictly serially and waited for natural join completion; post-fix all nine drains enter in the same millisecond, cancelled joins collapse in ~1.5 s, failed-SetUp teardown completes in 2.5 s.
  • Under a 20 s artificial worker-pool saturation (the CI-shape wedge): pre-fix teardown stayed wedged for the entire window; post-fix it completes ~5 s after the first pool liveness.
  • GuardedAsyncScopeTest adds the deadlock-shape regression test (drain that only completes if a sibling stop can progress — hangs with blocking close()).
  • dht end-to-end join suite (7 tests) passes with the cancellation plumbing under heavy machine load; utils and discovery unit tests pass.

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

ptesavol and others added 7 commits July 17, 2026 08:10
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>
@ptesavol
ptesavol force-pushed the claude/fix-full-node-teardown-hang branch from e2362a8 to d79c601 Compare July 17, 2026 07:09
@ptesavol

Copy link
Copy Markdown
Collaborator Author

Completed the teardown-hang fix: four more defects diagnosed and fixed on this branch

The 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 (SharedExecutors pinned to 4 threads, the 3-core runner size) showed 12 of 16 local runs failing, each layer hiding the next. All diagnosed with thread samples / crash reports before fixing; the branch is rebased onto main (picks up #93) and now carries six commits:

  1. ContentDeliveryManager (ceac17da): destroy()'s blocking joinScope.close() drained un-cancelled startLayersAndJoinDht/handleEntryPointLeave tasks — the same mechanism this PR fixed in NetworkStack, one layer down (sampled at ContentDeliveryManager.cppm:209). Same fix: cancellation source + closeAsync().
  2. StoreManager (149b2fef): detached onContactAdded replications launched with a bare .start(); they pinned the StoreManager but sent through the DhtNode's communicator, which died first (crash reports: notify<ReplicateDataRequest> resume, 13/16 once teardown got faster). Now scoped, cancelled, and drained in destroy() while the communicator is alive.
  3. ConnectionLocker (520ed7f2): lockConnection/unlockConnection blockingWait'ed their RPC round-trip on pool threads — with ≥poolsize concurrent joins inside lockConnection, every worker waits for a response only those workers could process (sampled: all four StreamrWorkers wedged). Interface and implementation are now coroutines; ProxyClient keeps its synchronous behavior by wrapping at its call sites (async-ifying that path is a follow-up).
  4. DhtNode (7a3a72ab): the server-RPC dispatch scope drained only at member destruction, after later-declared members were gone (crash reports: getClosestPeers dispatch resuming into freed memory). stop() now removes transport listeners with offAndWait() and drains the communicator last; the RPC APIs gate scope-adds after a drain.
  5. Websocket connectors (03a7f230): both destroy()s held the connector mutex across PendingConnection::close(true) call-outs — ABBA against rtc-callback/pool threads holding a connection mutex (sampled twice). Call-outs now run outside the lock.
  6. d79c601b: clang-format convergence (checker's llvm-22 binary); mostly reverts cosmetic reflow — review with whitespace hidden.

Validation (CI-emulated 4-thread pool, WebrtcFullNodeNetworkTest + WebsocketFullNodeNetworkTest): baseline this branch 12/16 failing → 15/16 passing, zero hangs. At normal pool sizes: proto-rpc suite 24/24, ConnectionLockingTest 2/2, both full-node tests 2/2 each, 64-node test 41 s, Layer1ScaleTest.MultipleLayer1Dht 326 s. Lint green on all three touched packages.

The one residual failure (1/16 under pathological load) is a distinct pre-existing UAF (neighbordiscovery::Handshaker::handshakeWithInterleaving resuming into a destroyed owner) — filed as a follow-up task chip together with an AbortableTimers lifetime bug spotted in the same runs.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant