diff --git a/selection.go b/selection.go index efed3353..9c760498 100644 --- a/selection.go +++ b/selection.go @@ -461,7 +461,15 @@ func (s *controlledSelector) HandleBindingRequest(message *stun.Message, local, } s.agent.sendBindingSuccess(message, local, remote) - s.PingCandidate(local, remote) + + // Only send a triggered check during ICE checking phase (RFC 8445 §7.3.1.4). + // Once the pair is established (succeeded + selected), sending a triggered check + // on every inbound request creates a ping-pong busy loop: the remote side responds + // and sends its own request, which triggers another check here, repeating at 1/RTT. + // After connection, consent freshness is maintained by checkKeepalive() on a timer. + if pair.state != CandidatePairStateSucceeded || s.agent.getSelectedPair() == nil { + s.PingCandidate(local, remote) + } if s.agent.userBindingRequestHandler != nil { if shouldSwitch := s.agent.userBindingRequestHandler(message, local, remote, pair); shouldSwitch { diff --git a/selection_test.go b/selection_test.go index a69db072..f4914572 100644 --- a/selection_test.go +++ b/selection_test.go @@ -367,6 +367,103 @@ func TestControlledSelector_HandleSuccessResponse_UnknownTxID(t *testing.T) { require.True(t, logger.warned, "expected Warnf to be called for unknown TransactionID (hitting !ok branch)") } +// TestControlledSelector_NoTriggeredCheckAfterConnected verifies that once a pair +// is in Succeeded state with a selected pair, HandleBindingRequest does NOT send +// a triggered check (PingCandidate). Before the fix, every inbound Binding Request +// unconditionally called PingCandidate, creating a ping-pong busy loop at 1/RTT. +func TestControlledSelector_NoTriggeredCheckAfterConnected(t *testing.T) { + agent := bareAgentForPing() + agent.log = logging.NewDefaultLoggerFactory().NewLogger("test") + agent.remoteUfrag = selectionTestRemoteUfrag + agent.localUfrag = selectionTestLocalUfrag + agent.remotePwd = selectionTestPassword + agent.localPwd = selectionTestPassword + agent.tieBreaker = 1 + agent.isControlling.Store(false) + agent.onConnected = make(chan struct{}) + agent.setSelector() + + selector, ok := agent.getSelector().(*controlledSelector) + require.True(t, ok, "expected controlledSelector") + + local := newPingNoIOCand() + local.candidateBase.networkType = NetworkTypeUDP4 + local.candidateBase.resolvedAddr = &net.UDPAddr{IP: net.ParseIP("192.168.1.1"), Port: 10000} + + remote := newPingNoIOCand() + remote.candidateBase.networkType = NetworkTypeUDP4 + remote.candidateBase.resolvedAddr = &net.UDPAddr{IP: net.ParseIP("192.168.1.2"), Port: 20000} + + pair := agent.addPair(local, remote) + pair.state = CandidatePairStateSucceeded + agent.setSelectedPair(pair) + + // Record requests sent before calling HandleBindingRequest. + sentBefore := pair.RequestsSent() + + // Build a STUN Binding Request (no USE-CANDIDATE). + msg, err := stun.Build(stun.BindingRequest, + stun.TransactionID, + stun.NewUsername(agent.localUfrag+":"+agent.remoteUfrag), + stun.NewShortTermIntegrity(agent.localPwd), + stun.Fingerprint, + ) + require.NoError(t, err) + + // Call HandleBindingRequest multiple times — simulates repeated inbound requests. + for range 10 { + selector.HandleBindingRequest(msg, local, remote) + } + + // No triggered checks should have been sent since the pair is already connected. + assert.Equal(t, sentBefore, pair.RequestsSent(), + "triggered check should not be sent for a succeeded+selected pair") +} + +// TestControlledSelector_TriggeredCheckDuringChecking verifies that a triggered +// check IS sent when the pair is not yet in Succeeded state (normal ICE checking). +func TestControlledSelector_TriggeredCheckDuringChecking(t *testing.T) { + agent := bareAgentForPing() + agent.log = logging.NewDefaultLoggerFactory().NewLogger("test") + agent.remoteUfrag = selectionTestRemoteUfrag + agent.localUfrag = selectionTestLocalUfrag + agent.remotePwd = selectionTestPassword + agent.localPwd = selectionTestPassword + agent.tieBreaker = 1 + agent.isControlling.Store(false) + agent.onConnected = make(chan struct{}) + agent.setSelector() + + selector, ok := agent.getSelector().(*controlledSelector) + require.True(t, ok, "expected controlledSelector") + + local := newPingNoIOCand() + local.candidateBase.networkType = NetworkTypeUDP4 + local.candidateBase.resolvedAddr = &net.UDPAddr{IP: net.ParseIP("192.168.1.1"), Port: 10000} + + remote := newPingNoIOCand() + remote.candidateBase.networkType = NetworkTypeUDP4 + remote.candidateBase.resolvedAddr = &net.UDPAddr{IP: net.ParseIP("192.168.1.2"), Port: 20000} + + pair := agent.addPair(local, remote) + // Pair is in Waiting state (not Succeeded), no selected pair — normal ICE checking. + + sentBefore := pair.RequestsSent() + + msg, err := stun.Build(stun.BindingRequest, + stun.TransactionID, + stun.NewUsername(agent.localUfrag+":"+agent.remoteUfrag), + stun.NewShortTermIntegrity(agent.localPwd), + stun.Fingerprint, + ) + require.NoError(t, err) + + selector.HandleBindingRequest(msg, local, remote) + + assert.Greater(t, pair.RequestsSent(), sentBefore, + "triggered check should be sent during ICE checking phase") +} + func TestAutomaticRenomination(t *testing.T) { //nolint:maintidx report := test.CheckRoutines(t) defer report()