Phase AA fixes to main (re-landing #70, which merged into its stack base)#71
Merged
Conversation
…akiness (#70) 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>
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this PR exists: #70 was stacked on #69's branch, and merging it landed it into that base branch (
claude/connection-peer-fixes) rather than into main — #69 had already been squash-merged to main separately, so the phase-AA fixes never reached main. This PR cherry-picks the #70 squash commit onto main. The resulting tree is bit-identical (git diffempty) to the fully verifiedclaude/connection-peer-fixestip, so all of #70's verification applies as-is: 25/25 + 16/16 isolated stress runs green under a hang watchdog, full unit (172) + integration suites green.See #70 for the full description of the three fixes (shared-pending-connection close on tie-break rejection; flat-buffer message merge on connecting-endpoint flush; teardown drain of detached pings) and the runtime-evidence diagnosis.
🤖 Generated with Claude Code