Phase A0: connection-layer locking discipline + simultaneous-connect convergence

[ptesavol]

Summary

Phase A0 of the trackerless-network completion plan: the connection-layer locking-discipline pass promised at the end of phase 0.3. It fixes the two diagnosed ABBA lock-order inversions that deadlock streamr-dht's connection layer under --gtest_repeat stress, plus four further concurrency defects the same stress loop surfaced. Every one of these is a race TypeScript's single-threaded runtime never hits.

Reproduction before this PR (deadlock/timeout within a few rounds):

./streamr-dht-test-integration \
  --gtest_filter='Simultaneous*:ConnectionManagerIntegration*' --gtest_repeat=50

The two ABBA inversions

1. Endpoint::mutex vs. the per-state mutexes. A send() on the main thread locked Endpoint::mutex then waited on a state mutex, while the simulator dispatcher thread ran a ConnectingEndpointState connected-handler that locked the state mutex then drove the endpoint back through Endpoint::mutex.

The endpoint state machine now runs under one recursive mutex owned by Endpoint, reached by the state classes through EndpointStateInterface::getStateMachineMutex() — the per-state mutexes are gone. Every public Endpoint operation is two-phase: transition under the mutex, then run the call-outs (event emits, connection/pending-connection close, container removal, replay-emitter handler registration) after releasing it. The states return their call-outs to Endpoint as deferred closures. Connection and pending-connection event handlers pin the endpoint through a weak reference and re-validate under the mutex that they still own the current connection before acting (an in-flight emit can outlive off(), and a ReplayEventEmitter can replay during on()).

2. ConnectionManager::endpointsMutex vs. Endpoint::mutex. acceptNewConnection held endpointsMutex and called into an endpoint (setConnecting), while Endpoint::handleDisconnect held the endpoint mutex and called removeSelfFromContainer() (which takes endpointsMutex).

ConnectionManager now holds endpointsMutex only for the container lookups/inserts; createConnection, onNewConnection, setConnecting and the endpoint queries in send()/getConnections() run outside it, and handleDisconnect → removeSelfFromContainer no longer runs under the endpoint mutex. The exists-check/insert in acceptNewConnection is done under a single lock hold so two racing accepts can't both insert.

Four more defects found by the same stress loop

• EventEmitter::emit moved args into every listener. The dispatch loop std::forward-ed the arguments into each handler, so with more than one listener the second and later ones received moved-from (empty) values — silent data loss for any multi-listener event carrying std::vector<std::byte>. Each listener now gets its own copy.
• EventEmitter replay on() invoked the listener under the handler mutex — an ABBA deadlock against a thread holding a user lock and calling off(). The replay now runs after the internal mutexes are released; the replay/live-dispatch decision stays atomic under the handler mutex, so a late listener still sees the stored event exactly once.
• waitForEvent leaked a once-listener capturing a stack waiter on the timeout/cancel path, so a later emit invoked the dangling listener and called setValue on a destroyed promise (a folly Promise.h "Check failed: state_" abort, seen in the teardown of a timed-out connection). The waiter is now heap-owned and co-owned by the listener, which is removed on every exit path.
• Simultaneous-connect convergence race (the received2 message loss, initially ~1 in 30 stress iterations). The acceptNewConnection tie-break replaced on getOfferer == remote alone — correct only under TS's single-threaded ordering. The connector's dispatcher thread can deliver the incoming connection before the main-thread send() registers the outgoing one, and the two nodes then settle on different physical connections. Fixed with three order-independent changes:
  • direction-aware tie-break — onNewConnection carries an isLocalInitiated flag; the winning connection is the offerer's regardless of which thread registered it first;
  • adoption sequence number — ConnectionManager assigns a monotonic sequence under endpointsMutex and the endpoint adopts only strictly-newer sequences, so a race between the initial and the replacing setConnecting can't re-adopt the loser;
  • no teardown of the losing endpoint — OutgoingHandshaker::onHandshakeResponse routed any error response straight to pendingConnection->close(), tearing the endpoint (and its buffered messages) down on a DUPLICATE_CONNECTION rejection. It now routes through handleFailure, which marks the pending connection replaceAsDuplicate() — silencing it so the peer's subsequent connection close cannot drive or tear down the endpoint that is about to receive the winning connection. Per-association FIFO guarantees the error precedes that close, so the silencing is always in place in time.

Tests

• New unit tests lock in the two library fixes: streamr-eventemitter (per-listener copy; replay exactly-once; replay holds no handler mutex — a two-thread test) and streamr-utils (waitForEvent removes its listener after both timeout and delivery).
• Opt-in STREAMR_DHT_TSAN CMake option for the connection-layer concurrency work. The vcpkg dependencies are not TSAN-instrumented, so a clean CI leg remains a follow-up (noted in the plan).

Test plan

• Acceptance stress loop --gtest_filter='Simultaneous*:ConnectionManagerIntegration*' --gtest_repeat=500 — zero failures (was deadlocking/timing out within a few rounds)
• ./test.sh — 348/348 pass
• ./lint.sh — clean (clangd 22)

Stacked on the merged #55 (phase 0.3); branched from updated main. Refs trackerless-network-completion-plan.md phase A0.

🤖 Generated with Claude Code

[cursor]

Bugbot is not enabled for this team, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.