Skip to content

Phase AA: fix the three concurrent-connect defects behind the join flakiness - #70

Merged
ptesavol merged 1 commit into
claude/connection-peer-fixesfrom
claude/phase-aa-connect-fixes
Jul 10, 2026
Merged

ptesavol merged 1 commit into
claude/connection-peer-fixesfrom
claude/phase-aa-connect-fixes

Conversation

@ptesavol

@ptesavol ptesavol commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #69 — review/merge that first; this diff shows only the phase-AA fixes.

Resolves the phase-AA concurrent-connect flakiness (NonEntryPointNodesCanJoin ~1/3 fail, CanJoinEvenIfANodeIsOffline ~20-30% fail, occasional teardown hangs). Root-caused with runtime instrumentation on both peers (per-decision logs with node ids and pending-connection identities, captured from failing runs) — it was three distinct defects, none of them the previously-suspected "two symmetric duplicate connections" problem. Every fix was made only after a log line proved the mechanism.

1. send() closed the connector-shared pending connection on tie-break rejection

The connectors deduplicate outgoing connects per target — a connect() while a handshake is in flight returns the same PendingConnection. So when the k-bucket ping and a discovery query raced to a new peer, both send()s held one shared pending connection; the second acceptNewConnection was rejected by the offerer tie-break, and send() then close(false)d the shared connection — tearing down the endpoint the first send had just created:

cache-HIT same pc → accept-REJECT → pc-close willEmitDisconnected=1
→ endpoint-down 1 ms after creation → ping SEND_FAILED → peer dropped

Each churn also buffer.clear()ed every other buffered message (captured: 5 requests buffered, only the last delivered). Fix: never close on rejection — fall through to the existing endpoint; a genuinely losing duplicate is torn down by the DUPLICATE_CONNECTION handshake protocol (replaceAsDuplicate), matching TS, which ignores the return value.

2. ConnectingEndpointState destroyed message boundaries on flush

It buffered messages into one flat byte vector and flushed them as one connection->send(), while the receiving onData() parses each delivered blob as one protobuf Message — concatenated messages merge (proto last-field-wins) and all but one RPC request silently vanish:

sender:   buffer-flush bytes=549 count=2 (one send)
receiver: blobBytes=549 msgBytes=295 (ONE message)

The lost ping then hit RPC_TIMEOUT after 5 s on a perfectly healthy connection. Fix: buffer one entry per message, flush one send per message (TS buffers an array the same way).

3. Teardown hang

The detached k-bucket ping tasks pin PeerManager via sharedFromThis, so DhtNode destroying its shared_ptr doesn't end them — ~RoutingRpcCommunicator then joined its executor while a dangling ping still used it (proven by sample of a live hung process). Fix: the ping is cancellable (co_withCancellation with PeerManager's abort token) and PeerManager::stop() drains pingExecutor.join() after the abort, outside the mutex.

Verification

  • 25/25 isolated runs green under a hang watchdog with instrumentation (12× NonEntryPoint, 8× offline, 5× simultaneous) and 16/16 after instrumentation removal.
  • Full unit (172) + integration (15) suites green.
  • Both formerly quarantined join tests are re-enabled (in the phase-A8 test file, landing with A8).
  • All instrumentation removed; the diff is exactly the three fixes + the plan document's phase-AA section rewritten as resolved (full diagnosis recorded there).

Two earlier model-driven attempts (send-level connect serialization; same-direction duplicate rejection) had failed because the real defects were in the connector dedup cache and the buffer framing — subsystems those hypotheses didn't cover.

🤖 Generated with Claude Code

…akiness

Root-caused with runtime instrumentation on both peers (per-decision logs
with node ids and pending-connection identities, captured from failing
runs of NonEntryPointNodesCanJoin); three distinct defects, each proven
by a cited log line before fixing:

1. ConnectionManager::send() closed the connector-shared pending
   connection when the offerer tie-break rejected a concurrent outgoing
   accept. The connectors deduplicate connects per target, so the
   k-bucket ping and a discovery query racing to a new peer share ONE
   PendingConnection; the loser's close(false) tore down the endpoint
   the winner had just created (endpoint down 1 ms after creation, the
   racing send failing SEND_FAILED, the peer dropped from the k-bucket)
   and discarded every message buffered on it. Now the rejected send
   falls through and uses the existing endpoint; a genuinely losing
   duplicate is torn down by the DUPLICATE_CONNECTION handshake protocol
   (replaceAsDuplicate), matching TS, which ignores the return value.

2. ConnectingEndpointState buffered outgoing messages into one flat byte
   vector and flushed them as ONE send, while the receiving onData()
   parses each delivered blob as ONE protobuf Message — two buffered
   messages merged into one corrupt message (captured: flush bytes=549
   count=2 -> receiver blobBytes=549 msgBytes=295) and all but one RPC
   request silently vanished, timing out 5 s later on a healthy
   connection. The buffer now keeps one entry per message and flushes
   one send per message, as TS does.

3. Teardown hang: the detached k-bucket ping tasks pin PeerManager via
   sharedFromThis, so DhtNode destroying its shared_ptr does not end
   them, and ~RoutingRpcCommunicator joined its executor while a
   dangling ping still used it (proven by sampling a live hung run).
   The ping is now cancellable (co_withCancellation with PeerManager's
   abort token) and PeerManager::stop() drains pingExecutor after the
   abort, outside the mutex.

Verification: 25/25 isolated runs green under a hang watchdog before
instrumentation removal (12x NonEntryPointNodesCanJoin, 8x
CanJoinEvenIfANodeIsOffline, 5x CanJoinSimultaneously) and 16/16 after;
full unit (172) + integration (15) suites green. Both formerly
quarantined join tests are re-enabled (in the phase A8 test file, which
lands with A8). The plan document's phase AA section records the full
diagnosis.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Jul 9, 2026

Copy link
Copy Markdown

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.

@ptesavol
ptesavol merged commit fe91b17 into claude/connection-peer-fixes Jul 10, 2026
1 check passed
ptesavol added a commit that referenced this pull request Jul 10, 2026
…akiness (#70) (#71)

Root-caused with runtime instrumentation on both peers (per-decision logs
with node ids and pending-connection identities, captured from failing
runs of NonEntryPointNodesCanJoin); three distinct defects, each proven
by a cited log line before fixing:

1. ConnectionManager::send() closed the connector-shared pending
   connection when the offerer tie-break rejected a concurrent outgoing
   accept. The connectors deduplicate connects per target, so the
   k-bucket ping and a discovery query racing to a new peer share ONE
   PendingConnection; the loser's close(false) tore down the endpoint
   the winner had just created (endpoint down 1 ms after creation, the
   racing send failing SEND_FAILED, the peer dropped from the k-bucket)
   and discarded every message buffered on it. Now the rejected send
   falls through and uses the existing endpoint; a genuinely losing
   duplicate is torn down by the DUPLICATE_CONNECTION handshake protocol
   (replaceAsDuplicate), matching TS, which ignores the return value.

2. ConnectingEndpointState buffered outgoing messages into one flat byte
   vector and flushed them as ONE send, while the receiving onData()
   parses each delivered blob as ONE protobuf Message — two buffered
   messages merged into one corrupt message (captured: flush bytes=549
   count=2 -> receiver blobBytes=549 msgBytes=295) and all but one RPC
   request silently vanished, timing out 5 s later on a healthy
   connection. The buffer now keeps one entry per message and flushes
   one send per message, as TS does.

3. Teardown hang: the detached k-bucket ping tasks pin PeerManager via
   sharedFromThis, so DhtNode destroying its shared_ptr does not end
   them, and ~RoutingRpcCommunicator joined its executor while a
   dangling ping still used it (proven by sampling a live hung run).
   The ping is now cancellable (co_withCancellation with PeerManager's
   abort token) and PeerManager::stop() drains pingExecutor after the
   abort, outside the mutex.

Verification: 25/25 isolated runs green under a hang watchdog before
instrumentation removal (12x NonEntryPointNodesCanJoin, 8x
CanJoinEvenIfANodeIsOffline, 5x CanJoinSimultaneously) and 16/16 after;
full unit (172) + integration (15) suites green. Both formerly
quarantined join tests are re-enabled (in the phase A8 test file, which
lands with A8). The plan document's phase AA section records the full
diagnosis.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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