Fix STUN busy loop in controlled agent's HandleBindingRequest

[Star-ho]

Summary

The controlled selector's HandleBindingRequest unconditionally calls PingCandidate (triggered check) for every inbound Binding Request, even after ICE connectivity is fully established. This creates a STUN ping-pong busy loop between the controlled and controlling agents that repeats at 1/RTT speed.

The Problem

When the controlling agent sends keepalive Binding Requests to the controlled agent:

1. Controlled agent receives the Request
2. Controlled agent sends a Success Response (correct)
3. Controlled agent sends its own Binding Request back via PingCandidate (unnecessary after connection)
4. Controlling agent receives the Request, responds, and sends its next Request
5. Repeat from step 1

This loop runs continuously at network RTT speed:

RTT	Requests/sec per session
1.5ms (same DC)	~643
13ms (cross DC)	~76

With 200 concurrent WebRTC sessions, total STUN traffic reaches ~57,600 req/s instead of the expected ~100 req/s, causing kernel softirq CPU spikes (up to 70%) on the media server.

The Fix

Skip the triggered check when the candidate pair has already succeeded and a selected pair exists. During ICE checking phase, triggered checks are still sent per RFC 8445 §7.3.1.4. After connection, consent freshness is handled by checkKeepalive() on a timer.

// Before: unconditional triggered check
s.PingCandidate(local, remote)

// After: only during ICE checking phase
if !(pair.state == CandidatePairStateSucceeded && s.agent.getSelectedPair() != nil) {
    s.PingCandidate(local, remote)
}

Observed Impact

Metric	Before	After
STUN req/s per session	76–643	~0.5 (keepalive only)
200-session total	~57,600 req/s	~100 req/s
Reduction	—	99.8%

How to Reproduce

1. Set up a controlling agent (e.g., OvenMediaEngine) and a controlled agent using pion/ice
2. Establish 100+ concurrent WebRTC sessions between them
3. Monitor STUN RequestsSent from GetCandidatePairsStats() — the rate scales with 1/RTT instead of staying at the keepalive interval

Notes

• This bug exists since commit 165d04e (May 2019) when STUN Binding Indications were replaced with Binding Requests for RFC 7675 consent freshness
• Single-session impact is small (~76–643 req/s), but it compounds linearly with concurrent session count
• The controlling selector's HandleBindingRequest does not have this issue — it does not call PingCandidate

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.29%. Comparing base (ce7d392) to head (d71dcf7).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #907      +/-   ##
==========================================
+ Coverage   88.25%   88.29%   +0.03%     
==========================================
  Files          45       45              
  Lines        5740     5741       +1     
==========================================
+ Hits         5066     5069       +3     
+ Misses        462      461       -1     
+ Partials      212      211       -1     

Flag	Coverage Δ
go	88.29% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JoTurk]

Good catch, Thank you so much.

Do you mind adding a regression test so we don't break this again? and fix the lint.

Thanks :)

[Star-ho]

Thanks for the review! I've addressed your feedback:

• Lint fix: Applied De Morgan's law to satisfy staticcheck QF1001 (!(A && B) → A != ... || B == ...)
• Commit message: Shortened the subject line to under 50 characters
• Regression tests added:
  • TestControlledSelector_NoTriggeredCheckAfterConnected — verifies no triggered check is sent for a succeeded+selected pair (the busy loop scenario)
  • TestControlledSelector_TriggeredCheckDuringChecking — verifies triggered checks still work during the normal ICE checking phase

[erivni]

@Star-ho, great catch!
A question, did you have a chance to test this with ICE Lite? Per RFC 8445 §2.5, a lite implementation must not initiate connectivity checks, so shouldn't we skip PingCandidate entirely in that case?
I ran into this with AWS Kinesis WebRTC - there's still an infinite loop risk during the initial handshake where both sides keep bouncing binding requests back and forth and the connection never reaches Connected.

[JoTurk]

@erivni Our lite implementation isn't yet complete, other than this startup ping behavior in Lite mode that we should fix, we don't support lite controlling agent [1], so if Pion is both controlling and lite we fall-back to full ICE, it's in my todo-list to implement this, but any PRs are welcome :)

1. ice/selection.go

   Line 490 in 23e8708

   // TODO: implement lite controlling agent. For now falling back to full agent.

[Star-ho]

Thanks @erivni, @JoTurk 🙏

You're both right — re-reading RFC 8445 §2.5 ("Lite agents … do not generate connectivity checks") together with §6.1.1 makes it clear that guarding PingCandidate on agent.lite in the controlled path is the correct fix, and @JoTurk's note (lite controlling currently falls back to full ICE) lines up with that.

My setup only exercises the full-to-full path, so I couldn't reproduce the handshake loop locally. If either of you picks this up — or @erivni can share a minimal repro / pcap from the AWS Kinesis case — I'm happy to help review.

[erivni]

please see: #909