Add gathering generation extension to candidates #874

boushley · 2026-01-09T20:12:01Z

Description

Populate the generation extension on candidates added to ICE Agent.

boushley · 2026-01-09T20:13:52Z

agent.go

+		if prevState == GatheringStateGathering {
+			a.candidateNotifier.EnqueueCandidate(nil)
+		}


This is a slight departure from the previous behavior that was part of the base patch from @JoTurk

I'm not sure if this behavior (only send nil candidate when transitioning from Gathering to Complete) is more correct, or if we should keep the original behavior (send nil candidate anytime we transition into the Complete state)

yeah, the reason i added this is because i just wanted to make the test pass (because the previous pr did close => complete), but sending nil regardless is correct i think.

this is because we don't go to complete from close, like in the previous pr.

If I'm reading this right it looks like the previous behavior of sending the nil candidate anytime we enter Complete as long as the previous state wasn't also Complete is in line with Chrome's behavior.

boushley · 2026-01-09T20:16:56Z

Converted to draft since I missed that I should be piping generation through the call stack from Agent#GatherCandidates instead of pulling fresh from agent within the individual gather methods. I'll update that.

codecov · 2026-01-09T20:18:33Z

Codecov Report

❌ Patch coverage is 75.94937% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.18%. Comparing base (4da5bc1) to head (d5aaf7d).

Files with missing lines	Patch %	Lines
gather.go	67.50%	7 Missing and 6 partials ⚠️
agent.go	84.61%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #874      +/-   ##
==========================================
- Coverage   88.27%   88.18%   -0.09%     
==========================================
  Files          43       43              
  Lines        5509     5544      +35     
==========================================
+ Hits         4863     4889      +26     
- Misses        451      458       +7     
- Partials      195      197       +2

Flag	Coverage Δ
go	`88.18% <75.94%> (-0.09%)`	⬇️

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 · 2026-01-09T20:22:42Z

Thank you I'll check again how chrome and firefox handle generations and review this. this pr is continuation of pion/webrtc#3113

boushley · 2026-01-09T21:29:14Z

gather.go

+					generationChan <- newGen
+				})
+				generation := <- generationChan
+				a.gatherCandidatesInternal(ctx, generation)


@JoTurk thoughts on this? It feels a bit janky, but I wasn't sure of a cleaner way. I'm assuming that re-triggering candidate gathering because of a network change should trigger a new generation. If not then I can revert this, but if so I'm open to recommendations on cleaner ways to run the bump and then gather candidates again, I didn't want to run the actual gatherCandidatesInternal method within the agent lock/loop as that seems like a major change.

I'm not entirely sure, since browsers don't do continual gathering and there isn't really a standard behavior to reference here. But I think this makes sense, we don't want the remote side attempting to connect to candidates before a network change. Also it's a way to signal that a gather did happen and how many gathers happend.

Kinda relates to this pion/webrtc#3323

I don't think we should bump the generation during continual gathering. Network changes can be additive, for example, if a peer on a mobile network connects to wifi, a new path becomes available while the old one remains valid. We want to allow the ICE agent to smoothly renominate the better path without tearing down the session.

My understanding is that generations are useful for hard resets, like ice restarts where credentials change, rendering previous candidates invalid. Since continual gathering uses existing credentials, the generation id must remain the same.

@wrangelvid Generations aren't used to tear down or invalidate already-accepted candidates. They're used to reject new candidates from an older context when we call AddCandidates. In this module, once a candidate has been accepted, bumping the generation won't affect it at all so nothing to worry about.

During a network change we'll still generate candidates for all available paths, including additive, and the ICE agent can continue to smoothly renominate the better path without disrupting the existing session. It will just make sure to reject old generation candidates if they got sent late. and not waste time try to connect to most likely not working candidates.

Mhh, I might need a bit more help to understand this, but I don't see the advantage yet.
My understanding is that the receiver discards identical candidates automatically. Wouldn't bumping the generation break this deduplication and trigger redundant connectivity checks for paths that are already connected?

Regarding late candidates: if a candidate from the current session arrives late, wouldn't we actually want to accept it? Since the credentials haven't changed, it's a valid backup path that just arrived out of order. I thought we only needed to strictly reject late candidates during ICE restarts because the credentials become invalid.

Also, if an interface is truly gone (e.g. unplugged ethernet), we won't gather new candidates for it anyway, so there's nothing to filter there.

We have to decide here:

Reject older generation candidates and save network time on most likely not valid network paths. and risk slight delay if we reject a valid candidate (we'll end up receving it again with the new generation anyway).

Accept all candidates, in case some of the candidates are valid, and risk wasting a lot of time in cases the user switch between different networks really quick.

I think your scenario will be super rare in the real world, what will be most likely the case is like in whip/whep signaling where we collect many candidates and batch them in one response as in the original issue pion/webrtc#2993 and in cases like this we have candidates from both generations.

Fair enough, that's a trade-off. We'll try it with generations on mobile robots when we get a chance and compare. Worst case, we'll vendor patch this.

Can always remove the generation extension from the candidate text, if you have a non standard signaling protocol :)

Well, we should keep the generation extension for ice restarts ◡̈

Alright, seems like we've landed on trying the generation bump on network change for now. We can certainly reevaluate in the future.

With that agreement in place are there thoughts on whether there's a better way to be executing that on the Agent Loop and getting the generation value back to this go-routine?

Populate the generation extension on candidates added to ICE Agent.

boushley · 2026-01-11T04:21:01Z

agent.go

+	}
+
+	if newState == GatheringStateComplete && !a.gatherEndSent {
+		a.candidateNotifier.EnqueueCandidate(nil)


@JoTurk I wonder if we should also be concerned about calling this from within the "Lock" / AgentLoop here. Specifically the fact that EnqueueCandidate starts by acquiring the handlerNotifier Lock. I'd be concerned that we could end up delaying the Agent Loop if not fully deadlocking it.

boushley commented Jan 9, 2026

View reviewed changes

boushley marked this pull request as draft January 9, 2026 20:15

boushley force-pushed the add-gathering-generation branch 2 times, most recently from 8b913a0 to 6f163a5 Compare January 9, 2026 21:25

boushley commented Jan 9, 2026

View reviewed changes

boushley force-pushed the add-gathering-generation branch from 6f163a5 to ecd2263 Compare January 9, 2026 21:42

Add gathering generation extension to candidates

d5aaf7d

Populate the generation extension on candidates added to ICE Agent.

boushley force-pushed the add-gathering-generation branch from ecd2263 to d5aaf7d Compare January 11, 2026 04:10

boushley commented Jan 11, 2026

View reviewed changes

boushley marked this pull request as ready for review January 12, 2026 18:32

Add gathering generation extension to candidates #874

Are you sure you want to change the base?

Add gathering generation extension to candidates #874

Conversation

boushley commented Jan 9, 2026

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoTurk Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boushley commented Jan 9, 2026

Uh oh!

codecov bot commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JoTurk commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoTurk Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

JoTurk Jan 9, 2026 •

edited

Loading

codecov bot commented Jan 9, 2026 •

edited

Loading

JoTurk commented Jan 9, 2026 •

edited

Loading

JoTurk Jan 12, 2026 •

edited

Loading