mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-08 14:39:09 +02:00
Riccardo Manfrin
5433e92183
* [client] Pass the home relay client into isForeignServer isForeignServer read m.relayClient directly, so it was only safe while the caller held relayClientMu, and that dependency was invisible at the call site. Taking the client as a parameter makes the requirement explicit and lets a caller narrow the window it holds the lock for. Pure refactor, no behavior change: the single caller passes m.relayClient while still holding the read lock. * [client] Release the relay client lock before dialing a foreign server OpenConn held relayClientMu for its whole body, dial included. Dialing a foreign relay takes as long as that server's connect timeout, and onServerDisconnected needs the same mutex in write mode; a pending writer also blocks later readers, so a single unreachable relay stalled every relay operation on the node for the duration of the dial. On a routing peer this stalled the disconnect handling of an unrelated relay for a full dial timeout. By the time the eviction ran, the track had already been rebuilt, so it dropped a working client and the next OpenConn built a duplicate connection to a server the peer was already connected to. Read the home client under the lock, then release it and dial unlocked. * [client] Keep a reconnected foreign relay client on a late disconnect notice evictForeignRelay deleted the track by server address without checking which client the entry held. The disconnect notice is delivered on its own goroutine and carries only the address, so it can arrive after the track has been rebuilt: the eviction then dropped a live client, and the next OpenConn built a second connection to a server the peer was already connected to. The relay answers a duplicate peer ID by closing the existing connection, which tears down every relayed channel on it, including the peers whose active path it is. Skip the eviction when the stored client is still connected. * [client] Drop the comments added with the previous commits Comment-only change, no behavior change. * [client] Cover the foreign relay stall and the late disconnect notice Two regression tests, both failing on the unfixed code: TestOpenConn_DoesNotHoldRelayClientLockAcrossDial dials a listener that accepts and never answers, then calls onServerDisconnected and fails if it does not return. On the unfixed code the writer waits out the whole dial. TestEvictForeignRelay_KeepsConnectedClient opens a real connection to a second relay and then replays a disconnect notice for that address. On the unfixed code the live client is dropped from the map. Both reuse the existing stallingRelayListener and relay server helpers. * [client] Keep a foreign relay track whose dial is still in progress openConnVia publishes the track before dialing and fills relayClient only once the dial finishes, so the readiness guard did not cover the dial-in-progress window: a disconnect notice arriving there deleted a track whose client was about to connect. That client then became unreachable through the map, so cleanUpUnusedRelays could not close it either, and the next OpenConn dialed a duplicate the relay answers by closing the first - the churn this branch is meant to remove. Skip the eviction while relayClient is still nil, the same signal cleanUpUnusedRelays already uses to leave an in-flight dial alone. Reported by cubic on PR #8098.