Skip to content

Ignore malformed nomination attributes#939

Merged
JoTurk merged 1 commit into
mainfrom
jo/fix-invalid-use-candidate
Jul 13, 2026
Merged

Ignore malformed nomination attributes#939
JoTurk merged 1 commit into
mainfrom
jo/fix-invalid-use-candidate

Conversation

@JoTurk

@JoTurk JoTurk commented Jul 13, 2026

Copy link
Copy Markdown
Member

Description

pion/ice had this bug for a while where it enter controlled nomination handling when we receive a malformed nomination-only binding request even without use-candidate. This isn't a big issue because we used connectivity checks to promote waiting candidates, but after #909 we don't do checks for LITE and we promote them directly to Succeeded, and this can cause problems with non standard ICE clients or clients that send malformed nominations.
I found this by looking at the selection code while reviewing and fixing #909 and it deserved its own PR / commit because it wasn't directly related.

@JoTurk JoTurk marked this pull request as ready for review July 13, 2026 04:16
@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #939      +/-   ##
==========================================
+ Coverage   88.35%   88.56%   +0.20%     
==========================================
  Files          46       46              
  Lines        5978     5982       +4     
==========================================
+ Hits         5282     5298      +16     
+ Misses        481      470      -11     
+ Partials      215      214       -1     
Flag Coverage Δ
go 88.56% <100.00%> (+0.20%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 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 JoTurk force-pushed the jo/fix-invalid-use-candidate branch from 76eca6b to bcd6915 Compare July 13, 2026 04:21
@JoTurk JoTurk requested a review from Copilot July 13, 2026 04:21

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a long-standing issue where the controlled agent would treat presence of the nomination attribute as a nomination even when that attribute was malformed (and even without USE-CANDIDATE). This matters more after #909 because lite controlled agents can now directly promote pairs to Succeeded on nomination, so malformed nomination-only requests could incorrectly select a pair.

Changes:

  • Only treat the custom nomination attribute as a nomination trigger when it parses successfully (valid length/value extraction).
  • Keep USE-CANDIDATE behavior unchanged, even if a malformed nomination attribute is also present.
  • Add regression tests covering malformed nomination-only requests vs USE-CANDIDATE + malformed nomination.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
selection.go Requires a validly parsed nomination attribute before entering nomination handling; continues to accept USE-CANDIDATE nominations.
selection_test.go Adds lite-mode regression tests ensuring malformed nomination-only requests do not nominate/select, while USE-CANDIDATE still does.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@JoTurk

JoTurk commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

@kevmo314 thank you for the fast review :)

@JoTurk JoTurk merged commit bcd6915 into main Jul 13, 2026
20 checks passed
@JoTurk JoTurk deleted the jo/fix-invalid-use-candidate branch July 13, 2026 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants