From a28c76982b17e3608140ee3c882932eebcd18467 Mon Sep 17 00:00:00 2001 From: sukun Date: Wed, 22 Jul 2026 21:11:19 +0530 Subject: [PATCH 1/8] fix(basichost): log updated host addrs, not local addrs 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 --- p2p/host/basic/addrs_manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/p2p/host/basic/addrs_manager.go b/p2p/host/basic/addrs_manager.go index cc90de4012..3abeec22c2 100644 --- a/p2p/host/basic/addrs_manager.go +++ b/p2p/host/basic/addrs_manager.go @@ -413,7 +413,7 @@ func (a *addrsManager) notifyAddrsUpdated(emitter event.Emitter, localAddrsEmitt } } if areAddrsDifferent(previous.addrs, current.addrs) { - log.Debug("host addresses updated", "addrs", current.localAddrs) + log.Debug("host addresses updated", "addrs", current.addrs) a.emitLocalAddrsUpdated(localAddrsEmitter, current.addrs, previous.addrs) } From aaa0333a0c286f1aa7b8f9fdb1f396484195afb3 Mon Sep 17 00:00:00 2001 From: sukun Date: Wed, 22 Jul 2026 21:34:15 +0530 Subject: [PATCH 2/8] fix(basichost): score webrtc-direct addrs with P_WEBRTC_DIRECT 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 --- p2p/host/basic/addrs_reachability_tracker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/p2p/host/basic/addrs_reachability_tracker.go b/p2p/host/basic/addrs_reachability_tracker.go index 5e68847df4..29ebfe4fd1 100644 --- a/p2p/host/basic/addrs_reachability_tracker.go +++ b/p2p/host/basic/addrs_reachability_tracker.go @@ -834,7 +834,7 @@ func assignPrimaryAddrs(statuses map[string]*addrStatus) { score += 1 case ma.P_WEBTRANSPORT: score += 1 << 1 - case ma.P_WEBRTC: + case ma.P_WEBRTC_DIRECT: score += 1 << 2 case ma.P_WS, ma.P_WSS: score += 1 << 3 From de127255d3a3cf8879ca638d0296f4b7cdd445b5 Mon Sep 17 00:00:00 2001 From: sukun Date: Wed, 22 Jul 2026 21:34:47 +0530 Subject: [PATCH 3/8] test(basichost): actually spawn workers in refreshReachability cancellation 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 --- p2p/host/basic/addrs_reachability_tracker_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/p2p/host/basic/addrs_reachability_tracker_test.go b/p2p/host/basic/addrs_reachability_tracker_test.go index 5e88e6c489..d52d4d3175 100644 --- a/p2p/host/basic/addrs_reachability_tracker_test.go +++ b/p2p/host/basic/addrs_reachability_tracker_test.go @@ -658,11 +658,12 @@ func TestRefreshReachability(t *testing.T) { pm := newProbeManager(time.Now) pm.UpdateAddrs([]ma.Multiaddr{pub1}) r := &addrsReachabilityTracker{ - ctx: ctx, - cancel: cancel, - client: mockClient, - probeManager: pm, - clock: clock.New(), + ctx: ctx, + cancel: cancel, + client: mockClient, + probeManager: pm, + clock: clock.New(), + maxConcurrency: defaultMaxConcurrency, } var wg sync.WaitGroup wg.Add(1) From 7ccb3cdd9b50630a4180e124f306f145f65fb402 Mon Sep 17 00:00:00 2001 From: sukun Date: Wed, 22 Jul 2026 21:36:03 +0530 Subject: [PATCH 4/8] fix(autonatv2): don't panic in GetReachability after Close 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 --- p2p/protocol/autonatv2/autonat.go | 7 +++++++ p2p/protocol/autonatv2/autonat_test.go | 10 ++++++++++ 2 files changed, 17 insertions(+) diff --git a/p2p/protocol/autonatv2/autonat.go b/p2p/protocol/autonatv2/autonat.go index 95f78c9329..e9ad09151e 100644 --- a/p2p/protocol/autonatv2/autonat.go +++ b/p2p/protocol/autonatv2/autonat.go @@ -174,7 +174,9 @@ func (an *AutoNAT) Close() { an.wg.Wait() an.srv.Close() an.cli.Close() + an.mx.Lock() an.peers = nil + an.mx.Unlock() } // GetReachability makes a single dial request for checking reachability for requested addresses @@ -196,6 +198,11 @@ func (an *AutoNAT) GetReachability(ctx context.Context, reqs []Request) (Result, filteredReqs = reqs } an.mx.Lock() + // nil after Close; host shutdown can have in-flight reachability checks + if an.peers == nil { + an.mx.Unlock() + return Result{}, ErrNoPeers + } now := time.Now() var p peer.ID for pr := range an.peers.Shuffled() { diff --git a/p2p/protocol/autonatv2/autonat_test.go b/p2p/protocol/autonatv2/autonat_test.go index dca55d2140..4bccb25404 100644 --- a/p2p/protocol/autonatv2/autonat_test.go +++ b/p2p/protocol/autonatv2/autonat_test.go @@ -97,6 +97,16 @@ func TestAutoNATPrivateAddr(t *testing.T) { require.ErrorIs(t, err, ErrPrivateAddrs) } +func TestGetReachabilityAfterClose(t *testing.T) { + // The host closes autonat before the address manager, whose reachability + // tracker workers may still issue checks during shutdown. + an := newAutoNAT(t, nil) + an.Close() + res, err := an.GetReachability(context.Background(), []Request{{Addr: ma.StringCast("/ip4/1.2.3.4/udp/10/quic-v1")}}) + require.ErrorIs(t, err, ErrNoPeers) + require.Equal(t, Result{}, res) +} + func TestClientRequest(t *testing.T) { an := newAutoNAT(t, nil, AllowPrivateAddrs) defer an.Close() From f667f336886cf3b8d6e76d8e6a151b93d23f9548 Mon Sep 17 00:00:00 2001 From: sukun Date: Wed, 22 Jul 2026 22:57:52 +0530 Subject: [PATCH 5/8] fix(basichost): probe secondary addrs on the same 1h cadence as primaries 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 --- p2p/host/basic/addrs_reachability_tracker.go | 7 +- .../basic/addrs_reachability_tracker_test.go | 110 ++++++++++++++---- 2 files changed, 89 insertions(+), 28 deletions(-) diff --git a/p2p/host/basic/addrs_reachability_tracker.go b/p2p/host/basic/addrs_reachability_tracker.go index 29ebfe4fd1..559645057d 100644 --- a/p2p/host/basic/addrs_reachability_tracker.go +++ b/p2p/host/basic/addrs_reachability_tracker.go @@ -363,9 +363,7 @@ const ( maxRecentDialsWindow = targetConfidence + 2 // highConfidenceAddrProbeInterval is the maximum interval between probes for an address highConfidenceAddrProbeInterval = 1 * time.Hour - // highConfidenceSecondaryAddrProbeInterval is the maximum interval between probes for an address - highConfidenceSecondaryAddrProbeInterval = 3 * time.Hour - // maxProbeResultTTL is the maximum time to keep probe results for a primary address + // maxProbeResultTTL is the maximum time to keep probe results for an address maxProbeResultTTL = maxRecentDialsWindow * highConfidenceAddrProbeInterval ) @@ -665,8 +663,7 @@ func (s *addrStatus) requiredProbeCountForConfirmation(now time.Time) int { } lastOutcome := s.outcomes[len(s.outcomes)-1] // If the last probe result is old, we need to retest - if d := now.Sub(lastOutcome.At); (s.primary == nil && d > highConfidenceAddrProbeInterval) || - (d > highConfidenceSecondaryAddrProbeInterval) { + if now.Sub(lastOutcome.At) > highConfidenceAddrProbeInterval { return 1 } // if the last probe result was different from reachability, probe again. diff --git a/p2p/host/basic/addrs_reachability_tracker_test.go b/p2p/host/basic/addrs_reachability_tracker_test.go index d52d4d3175..4e443088db 100644 --- a/p2p/host/basic/addrs_reachability_tracker_test.go +++ b/p2p/host/basic/addrs_reachability_tracker_test.go @@ -254,17 +254,21 @@ func TestProbeManager(t *testing.T) { matest.AssertMultiaddrsMatch(t, []ma.Multiaddr{tcp, websocket}, reachable) matest.AssertMultiaddrsMatch(t, []ma.Multiaddr{quic, webrtc}, unreachable) - // After highConfidenceAddrProbeInterval (1h), only primaries need refresh. - // websocket inherits from tcp (Public), webrtc has longer refresh interval (3h). + // After highConfidenceAddrProbeInterval (1h) every probed address needs a + // refresh: tcp and quic (primaries) plus webrtc (a secondary with a Private + // primary, so it doesn't inherit and refreshes on the same cadence). + // websocket inherits Public from tcp and is never probed. for range 2 { cl.Add(highConfidenceAddrProbeInterval + 1*time.Millisecond) reqs := nextProbe(pm) - // Only tcp and quic need refresh; websocket inherits, webrtc has 3h interval - matest.AssertEqualMultiaddrs(t, []ma.Multiaddr{tcp, quic}, extractAddrs(reqs)) + matest.AssertEqualMultiaddrs(t, []ma.Multiaddr{tcp, quic, webrtc}, extractAddrs(reqs)) pm.CompleteProbe(reqs, autonatv2.Result{Addr: tcp, Idx: 0, Reachability: network.ReachabilityPublic}, nil) reqs = nextProbe(pm) - matest.AssertEqualMultiaddrs(t, []ma.Multiaddr{quic}, extractAddrs(reqs)) + matest.AssertEqualMultiaddrs(t, []ma.Multiaddr{quic, webrtc}, extractAddrs(reqs)) pm.CompleteProbe(reqs, autonatv2.Result{Addr: quic, Idx: 0, Reachability: network.ReachabilityPrivate}, nil) + reqs = nextProbe(pm) + matest.AssertEqualMultiaddrs(t, []ma.Multiaddr{webrtc}, extractAddrs(reqs)) + pm.CompleteProbe(reqs, autonatv2.Result{Addr: webrtc, Idx: 0, Reachability: network.ReachabilityPrivate}, nil) reqs = nextProbe(pm) require.Empty(t, reqs) @@ -273,24 +277,6 @@ func TestProbeManager(t *testing.T) { reachable, unreachable, _ = pm.AppendConfirmedAddrs(nil, nil, nil) matest.AssertMultiaddrsMatch(t, reachable, []ma.Multiaddr{tcp, websocket}) matest.AssertMultiaddrsMatch(t, unreachable, []ma.Multiaddr{quic, webrtc}) - - // After highConfidenceSecondaryAddrProbeInterval (3h), webrtc needs refresh too. - // We've advanced 2h+2ms, need to reach 3h+ for webrtc's refresh. - // Also need to exceed 1h since last tcp/quic refresh for them to need refresh. - cl.Add(highConfidenceSecondaryAddrProbeInterval - 2*highConfidenceAddrProbeInterval + 1*time.Millisecond) - reqs = nextProbe(pm) - // tcp, quic, and webrtc need refresh; websocket still inherits from tcp - matest.AssertEqualMultiaddrs(t, []ma.Multiaddr{tcp, quic, webrtc}, extractAddrs(reqs)) - pm.CompleteProbe(reqs, autonatv2.Result{Addr: tcp, Idx: 0, Reachability: network.ReachabilityPublic}, nil) - reqs = nextProbe(pm) - matest.AssertEqualMultiaddrs(t, []ma.Multiaddr{quic, webrtc}, extractAddrs(reqs)) - pm.CompleteProbe(reqs, autonatv2.Result{Addr: quic, Idx: 0, Reachability: network.ReachabilityPrivate}, nil) - reqs = nextProbe(pm) - matest.AssertEqualMultiaddrs(t, []ma.Multiaddr{webrtc}, extractAddrs(reqs)) - pm.CompleteProbe(reqs, autonatv2.Result{Addr: webrtc, Idx: 0, Reachability: network.ReachabilityPrivate}, nil) - - reqs = nextProbe(pm) - require.Empty(t, reqs) }) } @@ -596,6 +582,84 @@ func TestAddrsReachabilityTracker(t *testing.T) { t.Fatal("expected probe") } }) + + t.Run("secondary addr stays confirmed across the result TTL", func(t *testing.T) { + // quic-v1 (primary) is unreachable, webrtc-direct (secondary sharing the + // socket) is reachable. Since the primary isn't Public the secondary + // doesn't inherit and keeps its own status, refreshed on the 1h cadence. + // Advancing well past maxProbeResultTTL (5h) must not drop it to Unknown: + // before the fix the secondary refreshed only every 3h and expired out of + // the 5h window. + quic := ma.StringCast("/ip4/1.1.1.1/udp/1/quic-v1") + webrtc := ma.StringCast("/ip4/1.1.1.1/udp/1/webrtc-direct") + + notify := make(chan struct{}, 1) + drainNotify := func() { + for { + select { + case <-notify: + default: + return + } + } + } + mockClient := mockAutoNATClient{ + F: func(_ context.Context, reqs []autonatv2.Request) (autonatv2.Result, error) { + select { + case notify <- struct{}{}: + default: + } + if reqs[0].Addr.Equal(webrtc) { + return autonatv2.Result{Addr: webrtc, Idx: 0, Reachability: network.ReachabilityPublic}, nil + } + return autonatv2.Result{Addr: reqs[0].Addr, Idx: 0, Reachability: network.ReachabilityPrivate}, nil + }, + } + + cl := clock.NewMock() + tr := newTracker(mockClient, cl) + tr.UpdateAddrs([]ma.Multiaddr{quic, webrtc}) + assertFirstEvent(t, tr, []ma.Multiaddr{quic, webrtc}) + + drainEvent := func() { + select { + case <-tr.reachabilityUpdateCh: + default: + } + } + + time.Sleep(100 * time.Millisecond) // let the background goroutine process the new addrs + cl.Add(1) // fire the initial probe timer + time.Sleep(100 * time.Millisecond) // let the probes run + drainNotify() + reachable, unreachable, _ := tr.ConfirmedAddrs() + require.Equal(t, []ma.Multiaddr{webrtc}, reachable) + require.Equal(t, []ma.Multiaddr{quic}, unreachable) + + // Refresh repeatedly past the 5h TTL. Each cycle advances just past the + // 1h refresh interval plus one ticker interval so a refresh fires. The + // confirmed reachability must not change: a flap to Unknown (before the + // fix) shows up as a spurious reachability-update event. + for range 6 { + drainNotify() + drainEvent() + cl.Add(highConfidenceAddrProbeInterval + defaultReachabilityRefreshInterval + time.Millisecond) + select { + case <-notify: + case <-time.After(1 * time.Second): + t.Fatal("expected a refresh probe") + } + time.Sleep(100 * time.Millisecond) // let the probe results settle + select { + case <-tr.reachabilityUpdateCh: + t.Fatal("unexpected reachability change: secondary flapped") + default: + } + reachable, _, unknown := tr.ConfirmedAddrs() + require.Equal(t, []ma.Multiaddr{webrtc}, reachable, "secondary must stay reachable") + require.Empty(t, unknown) + } + }) } func TestRefreshReachability(t *testing.T) { From 97e05cf4ea4e630a0f795f1452e29b0145f72e1b Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Fri, 24 Jul 2026 13:27:54 +0200 Subject: [PATCH 6/8] docs(basichost): fix stale const name in comments The comments on targetConfidence and maxRecentDialsWindow pointed at maxProbeInterval, which doesn't exist in the tree. The interval they describe is highConfidenceAddrProbeInterval. --- p2p/host/basic/addrs_reachability_tracker.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/p2p/host/basic/addrs_reachability_tracker.go b/p2p/host/basic/addrs_reachability_tracker.go index 559645057d..a98ed30a20 100644 --- a/p2p/host/basic/addrs_reachability_tracker.go +++ b/p2p/host/basic/addrs_reachability_tracker.go @@ -349,7 +349,7 @@ const ( // This is used to prevent infinite probing of an address whose status is indeterminate for any reason. maxRecentDialsPerAddr = 10 // confidence is the absolute difference between the number of successes and failures for an address - // targetConfidence is the confidence threshold for an address after which we wait for `maxProbeInterval` + // targetConfidence is the confidence threshold for an address after which we wait for `highConfidenceAddrProbeInterval` // before probing again. targetConfidence = 3 // minConfidence is the confidence threshold for an address to be considered reachable or unreachable @@ -359,7 +359,7 @@ const ( // // +2 allows for 1 invalid probe result. Consider a string of successes, after which we have a single failure // and then a success(...S S S S F S). The confidence in the targetConfidence window will be equal to - // targetConfidence, the last F and S cancel each other, and we won't probe again for maxProbeInterval. + // targetConfidence, the last F and S cancel each other, and we won't probe again for highConfidenceAddrProbeInterval. maxRecentDialsWindow = targetConfidence + 2 // highConfidenceAddrProbeInterval is the maximum interval between probes for an address highConfidenceAddrProbeInterval = 1 * time.Hour From 7ba8874bb408c6eb8f67c4cd1d49f8498f289f4c Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Fri, 24 Jul 2026 16:28:42 +0200 Subject: [PATCH 7/8] refactor(autonatv2): unlock pickServer via defer 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. --- p2p/protocol/autonatv2/autonat.go | 44 ++++++++++++++++++------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/p2p/protocol/autonatv2/autonat.go b/p2p/protocol/autonatv2/autonat.go index e9ad09151e..9aa04d45f6 100644 --- a/p2p/protocol/autonatv2/autonat.go +++ b/p2p/protocol/autonatv2/autonat.go @@ -197,25 +197,9 @@ func (an *AutoNAT) GetReachability(ctx context.Context, reqs []Request) (Result, } else { filteredReqs = reqs } - an.mx.Lock() - // nil after Close; host shutdown can have in-flight reachability checks - if an.peers == nil { - an.mx.Unlock() - return Result{}, ErrNoPeers - } - now := time.Now() - var p peer.ID - for pr := range an.peers.Shuffled() { - if t := an.throttlePeer[pr]; t.After(now) { - continue - } - p = pr - an.throttlePeer[p] = time.Now().Add(an.throttlePeerDuration) - break - } - an.mx.Unlock() - if p == "" { - return Result{}, ErrNoPeers + p, err := an.pickServer() + if err != nil { + return Result{}, err } res, err := an.cli.GetReachability(ctx, p, filteredReqs) if err != nil { @@ -233,6 +217,28 @@ func (an *AutoNAT) GetReachability(ctx context.Context, reqs []Request) (Result, return res, nil } +// pickServer returns an autonatv2 server that isn't currently throttled, and throttles +// it for throttlePeerDuration. It returns ErrNoPeers if there is no such server, or if +// autonat has been closed. +func (an *AutoNAT) pickServer() (peer.ID, error) { + an.mx.Lock() + defer an.mx.Unlock() + + // nil after Close; host shutdown can have in-flight reachability checks + if an.peers == nil { + return "", ErrNoPeers + } + now := time.Now() + for p := range an.peers.Shuffled() { + if t := an.throttlePeer[p]; t.After(now) { + continue + } + an.throttlePeer[p] = time.Now().Add(an.throttlePeerDuration) + return p, nil + } + return "", ErrNoPeers +} + func (an *AutoNAT) updatePeer(p peer.ID) { an.mx.Lock() defer an.mx.Unlock() From fad42536209e514dfa47cb98ff44d227ca98bae2 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Fri, 24 Jul 2026 16:28:42 +0200 Subject: [PATCH 8/8] test(basichost): cancel in-flight refresh probes 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. --- .../basic/addrs_reachability_tracker_test.go | 67 +++++++++++-------- 1 file changed, 40 insertions(+), 27 deletions(-) diff --git a/p2p/host/basic/addrs_reachability_tracker_test.go b/p2p/host/basic/addrs_reachability_tracker_test.go index 4e443088db..c3af935063 100644 --- a/p2p/host/basic/addrs_reachability_tracker_test.go +++ b/p2p/host/basic/addrs_reachability_tracker_test.go @@ -10,7 +10,6 @@ import ( "net/netip" "slices" "strings" - "sync" "sync/atomic" "testing" "time" @@ -710,17 +709,35 @@ func TestRefreshReachability(t *testing.T) { }) t.Run("quits on cancellation", func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - block := make(chan struct{}) + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + + // An addr per worker, so every worker holds a probe and a refresh that + // honors cancellation reaches the client exactly maxConcurrency times. + // (A fresh addr admits only targetConfidence concurrent probes, so a + // single addr couldn't guarantee a probe for every worker.) + addrs := make([]ma.Multiaddr, 0, defaultMaxConcurrency) + for i := range defaultMaxConcurrency { + addrs = append(addrs, ma.StringCast(fmt.Sprintf("/ip4/1.1.1.1/tcp/%d", i+1))) + } + + inFlight := make(chan struct{}, defaultMaxConcurrency) + var probes atomic.Int32 mockClient := mockAutoNATClient{ - F: func(_ context.Context, _ []autonatv2.Request) (autonatv2.Result, error) { - block <- struct{}{} - return autonatv2.Result{}, nil + F: func(ctx context.Context, _ []autonatv2.Request) (autonatv2.Result, error) { + // Checked on the test goroutine below. Failing here instead would + // risk logging to a t that has already completed. + if probes.Add(1) > defaultMaxConcurrency { + return autonatv2.Result{}, autonatv2.ErrNoPeers // persistent: unwinds the workers + } + inFlight <- struct{}{} + <-ctx.Done() // keep the probe in flight until it's cancelled + return autonatv2.Result{}, ctx.Err() }, } pm := newProbeManager(time.Now) - pm.UpdateAddrs([]ma.Multiaddr{pub1}) + pm.UpdateAddrs(addrs) r := &addrsReachabilityTracker{ ctx: ctx, cancel: cancel, @@ -729,32 +746,28 @@ func TestRefreshReachability(t *testing.T) { clock: clock.New(), maxConcurrency: defaultMaxConcurrency, } - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - result := r.refreshReachability() - assert.False(t, <-result.BackoffCh) - assert.Equal(t, pm.InProgressProbes(), 0) - }() - cancel() - time.Sleep(50 * time.Millisecond) // wait for the cancellation to be processed - - outer: + result := r.refreshReachability() + // Only cancel once every worker is parked inside the client, so cancellation + // has to interrupt probes that are genuinely running. for range defaultMaxConcurrency { select { - case <-block: - default: - break outer + case <-inFlight: + case <-time.After(5 * time.Second): + t.Fatal("expected every worker to have a probe in flight") } } + cancel() + select { - case <-block: - t.Fatal("expected no more requests") - case <-time.After(50 * time.Millisecond): - } - wg.Wait() + case backoff := <-result.BackoffCh: + // The workers are done, so probes is final. + require.Equal(t, int32(defaultMaxConcurrency), probes.Load(), "started a new probe after cancellation") + require.False(t, backoff) + case <-time.After(5 * time.Second): + t.Fatal("refreshReachability didn't return after cancellation") + } + require.Equal(t, 0, pm.InProgressProbes()) }) t.Run("handles refusals", func(t *testing.T) {