fix(basichost): fix some bugs in addrs reachability manager - #3528
Merged
Conversation
The "host addresses updated" debug log is guarded by a change in current.addrs but printed current.localAddrs, showing an unchanged list whenever only relay addrs or the addrs factory output changed. Assisted-By: Claude Fable 5
assignPrimaryAddrs matched ma.P_WEBRTC, which never appears in thin-waist addrs, so /webrtc-direct scored 0 and was classified as secondary only via the unknown-protocol fallback. Match the protocol actually used. No behavior change for the default transport set; ties against other unknown-protocol addrs now resolve deterministically. Assisted-By: Claude Fable 5
…lation test The "quits on cancellation" subtest constructed the tracker without maxConcurrency, so refreshReachability spawned zero workers, the mock client was never called, and the test passed vacuously. Assisted-By: Claude Fable 5
Close set an.peers = nil without holding an.mx while GetReachability reads it under the lock: an unsynchronized write, and a nil pointer panic in peersMap.Shuffled for callers racing with Close. The host closes autonat before the address manager, so the reachability tracker's probe workers can issue checks in exactly that window, crashing the process during shutdown. Guard the write with the mutex and return ErrNoPeers once closed; the reachability tracker treats ErrNoPeers as persistent and backs off its workers. Assisted-By: Claude Fable 5
…ries Secondary addresses (webrtc-direct, webtransport, wss sharing a socket with their thin-waist primary) refreshed every 3h while primaries refreshed every 1h. Probe results expire after maxProbeResultTTL (5 * 1h = 5h), which holds enough outcomes across a 1h cadence but not a 3h one: a secondary needs two live outcomes to stay above minConfidence, i.e. TTL >= 2 * refreshInterval. At 3h that is 6h > 5h, so a confirmed secondary periodically expired down to a single outcome and flapped to Unknown for up to a refresh-ticker interval every ~5h, emitting spurious reachability-change events and, for a confirmed- unreachable secondary, briefly re-advertising it in Addrs(). The 3h interval is vestigial. It was added in #3356 when secondaries were always probed (once, when the primary was confirmed) as a way to probe them less. #3435 then made secondaries inherit Public from their primary and skip probing entirely in that case, so the only secondaries still probed are those with a non-Public primary - exactly the ones we want kept fresh. The reduced-cadence rationale no longer applies. Drop highConfidenceSecondaryAddrProbeInterval and refresh every probed address at highConfidenceAddrProbeInterval, which collapses the primary/secondary branch in requiredProbeCountForConfirmation. Assisted-By: Claude Fable 5
sukunrt
force-pushed
the
sukun/autonatv2-fix
branch
from
July 22, 2026 18:24
f254253 to
f667f33
Compare
The comments on targetConfidence and maxRecentDialsWindow pointed at maxProbeInterval, which doesn't exist in the tree. The interval they describe is highConfidenceAddrProbeInterval.
GetReachability unlocked an.mx by hand before the network call, so a panic inside the critical section would leave the mutex held and any later Close() would block forever on an.mx.Lock, hiding the original panic behind a test-suite timeout. Extract the locked section into pickServer with a deferred unlock so a panic unwinds with the mutex released and surfaces as itself.
The "quits on cancellation" subtest raced cancel() against worker startup and consistently lost: every worker returned at the top-of-loop ctx check and the mock client was never entered, so cancellation of a running probe was never exercised. Park all maxConcurrency workers inside the client (one addr each), cancel only once every probe is in flight, and assert the probe count from the test goroutine once the workers are done.
lidel
approved these changes
Jul 24, 2026
Member
There was a problem hiding this comment.
Pushed two follow-ups from review:
-
7ba8874 refactor(autonatv2): unlock pickServer via defer
so a panic inside the locked section can't leavean.mxheld and wedgeClose() -
fad4253 test(basichost): cancel in-flight refresh probes
cancel() used to win the startup race, so the mock client was never entered; the subtest now parks every worker inside the client before cancelling
Otherwise, fixes look solid. @sukunrt LGTM, if it looks ok to you too, feel free to merge.
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
2 participants
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.
These are all minor except 7ccb3cd which may cause a panic on close.
All the commits are meaningful. See the commits for individual issues.