Skip to content

fix: Do not relay a transport the bridge did not restart - #1302

Merged
bgrozev merged 2 commits into
masterfrom
ice-restart-keep-existing-transport
Aug 17, 2026
Merged

fix: Do not relay a transport the bridge did not restart#1302
bgrozev merged 2 commits into
masterfrom
ice-restart-keep-existing-transport

Conversation

@bgrozev

@bgrozev bgrozev commented Aug 17, 2026

Copy link
Copy Markdown
Member

A transport with no ice-generation in a colibri2 response is not the answer to an ICE restart. The bridge sends it when it did not restart and described the Agent it already has, which it does when its own transport has not connected yet: there is nothing to preserve, and nothing for the participant to do.

ColibriV2SessionManager.endpointIceRestarted treated it as a relayed restart:

  • lastRelayedIceGeneration was set to GENERATION_UNSPECIFIED, resetting the high-water mark. The staleness guard then let a generation that had already been relayed through a second time.
  • ice_restarts_relayed counted a restart that never happened.
  • The participant was sent a transport-info it has nothing to do with. lib-jitsi-meet does not take the ICE restart path for an untagged transport, so it was ignored -- noise rather than a fault.

Now an absent generation is handled for what it is: log it, count it in the new ice_restarts_not_needed metric, relay nothing. The staleness check no longer needs to exclude untagged transports because they never reach it.

The existing test for untagged transports asserted the old behaviour and is replaced by two: the untagged transport is not relayed, and it does not lose the generation of the last one that was.

Independent of the bridge side, which is where these transports come from: jitsi/jitsi-videobridge#2438. Raised in review there: jitsi/jitsi-videobridge#2438 (comment)

A transport that carries no ice-generation is not the answer to an ICE
restart: the bridge did not restart and described the Agent it already
has, which it does when its transport has not connected yet. It was
treated as a relayed restart anyway, with three consequences:

- lastRelayedIceGeneration was set to GENERATION_UNSPECIFIED, which
  reset the high-water mark and lost the staleness protection for the
  next reordered response. A generation that had already been relayed
  could then be relayed again.
- ice_restarts_relayed counted a restart that never happened.
- The participant was sent a transport-info it has nothing to do with.
  It ignores it (an untagged transport does not take the ICE restart
  path in lib-jitsi-meet), so this was noise rather than a fault.

Treat an absent generation as "the bridge kept its existing transport":
log it, count it in the new ice_restarts_not_needed, and relay nothing.
The staleness check no longer has to exclude untagged transports, since
they never reach it.
@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.57%. Comparing base (d4d1458) to head (bb364c8).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1302      +/-   ##
==========================================
+ Coverage   51.53%   51.57%   +0.03%     
==========================================
  Files         132      132              
  Lines        8419     8425       +6     
  Branches     1285     1285              
==========================================
+ Hits         4339     4345       +6     
  Misses       3471     3471              
  Partials      609      609              
Files with missing lines Coverage Δ
...tlin/org/jitsi/jicofo/metrics/IceRestartMetrics.kt 92.30% <100.00%> (+2.30%) ⬆️
...org/jitsi/jicofo/bridge/colibri/Colibri2Session.kt 69.57% <ø> (ø)
...i/jicofo/bridge/colibri/ColibriV2SessionManager.kt 59.34% <100.00%> (+0.26%) ⬆️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4d1458...bb364c8. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JonathanLennox

Copy link
Copy Markdown
Member

Note: this comment was generated by AI (Claude Code), following on from the review comment on jitsi/jitsi-videobridge#2438 that prompted this PR. Please treat it as something to check rather than as established fact.

The change reads correctly to me, and ColibriV2SessionManagerTest passes locally (28 tests). Returning early before lastRelayedIceGeneration = generation is what preserves the high-water mark, and dropping the generation != GENERATION_UNSPECIFIED clause from the staleness check afterwards is right, since untagged transports can no longer reach it. The replacement test is well aimed at exactly the regression the old code had.

One thing left over, in a file this PR does not touch — Colibri2Session.kt:219-220:

// The bridge declined the restart (disabled in its configuration, transport not established yet,
// or shutting down). It says so by omitting the transport rather than by returning an error.

Both halves of that list are now out of date, in opposite directions:

  • "transport not established yet" no longer omits the transport. That is precisely the case where the bridge answers with the Agent it already has, so it now arrives as an untagged transport and is handled by the new branch this PR adds — not by endpointIceRestartFailed.
  • The bridge has since grown another way to omit the transport that is not listed: it answers UNAVAILABLE when it fails to create the new Agent (jitsi/jitsi-videobridge@77dd7ba7a). That one is a resource problem on the bridge rather than a configuration choice, and it is the case where escalating to a re-invite matters most.

Since this PR is specifically about pinning down which shape of response means what, that comment seems like the natural thing to correct alongside it — it is currently the only place in jicofo that documents the mapping, and it now points the reader at the wrong branch for the not-established case.

jitsi-ci Bot pushed a commit to jitsi/jitsi-pr-tests-pages that referenced this pull request Aug 17, 2026
The list of reasons was wrong in both directions. A transport that is
not established yet no longer omits the transport: the bridge answers
with the Agent it already has, which arrives as a transport with no
ice-generation and is handled by endpointIceRestarted. A failure to
create the new Agent does omit it, and was not listed.
@bgrozev
bgrozev merged commit e4831f1 into master Aug 17, 2026
2 checks passed
@bgrozev
bgrozev deleted the ice-restart-keep-existing-transport branch August 17, 2026 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants