From 2d4437684cc5b0cb19cbac294d677e612e3807fc Mon Sep 17 00:00:00 2001 From: Karol Konkol Date: Fri, 27 Jun 2025 18:05:38 +0200 Subject: [PATCH 1/8] Don't close candidate on eperm --- lib/ex_ice/priv/checklist.ex | 5 +---- lib/ex_ice/priv/ice_agent.ex | 2 +- test/priv/ice_agent_test.exs | 20 ++++++++++++++------ 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/lib/ex_ice/priv/checklist.ex b/lib/ex_ice/priv/checklist.ex index 54fcc4e..f5cc157 100644 --- a/lib/ex_ice/priv/checklist.ex +++ b/lib/ex_ice/priv/checklist.ex @@ -29,9 +29,6 @@ defmodule ExICE.Priv.Checklist do def get_valid_pair(checklist) do checklist |> Stream.map(fn {_id, pair} -> pair end) - # pair might have been marked as failed if the associated - # local candidate has been closed - |> Stream.filter(fn pair -> pair.state == :succeeded end) |> Stream.filter(fn pair -> pair.valid? end) |> Enum.sort_by(fn pair -> pair.priority end, :desc) |> Enum.at(0) @@ -89,7 +86,7 @@ defmodule ExICE.Priv.Checklist do def close_candidate(checklist, local_cand) do Enum.reduce(checklist, {[], checklist}, fn {pair_id, pair}, {failed_pair_ids, checklist} -> if pair.local_cand_id == local_cand.base.id and pair.state != :failed do - checklist = Map.put(checklist, pair_id, %{pair | state: :failed}) + checklist = Map.put(checklist, pair_id, %{pair | state: :failed, valid?: false}) {[pair_id | failed_pair_ids], checklist} else {failed_pair_ids, checklist} diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index a7e214b..8d00756 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/lib/ex_ice/priv/ice_agent.ex @@ -3085,7 +3085,7 @@ defmodule ExICE.Priv.ICEAgent do """) ice_agent = put_in(ice_agent.local_cands[local_cand.base.id], local_cand) - ice_agent = close_candidate(ice_agent, local_cand) + {:error, ice_agent} end end diff --git a/test/priv/ice_agent_test.exs b/test/priv/ice_agent_test.exs index 9fb8863..9ac63d2 100644 --- a/test/priv/ice_agent_test.exs +++ b/test/priv/ice_agent_test.exs @@ -2492,8 +2492,8 @@ defmodule ExICE.Priv.ICEAgentTest do test "candidate fails to send data when ice is connected" do # 1. make ice agent connected # 2. replace candidate with the mock one that always fails to send data - # 3. assert that after unsuccessful data sending, ice_agent moves to the failed state - # as there are no other pairs + # 3. assert that after unsuccessful data sending, ice_agent doesn't move to the failed state + # even when there is only one pair ice_agent = ICEAgent.new( @@ -2519,10 +2519,18 @@ defmodule ExICE.Priv.ICEAgentTest do # try to send some data ice_agent = ICEAgent.send_data(ice_agent, "somedata") - # assert that local cand has been closed and the agent moved to the failed state - assert [%{base: %{closed?: true}}] = Map.values(ice_agent.local_cands) - assert ice_agent.state == :failed - assert [%{state: :failed}] = Map.values(ice_agent.checklist) + # assert that the local candidate hasn't been closed and that the agent hasn't moved to a failed state + assert [%{base: %{closed?: false}}] = Map.values(ice_agent.local_cands) + assert ice_agent.state == :connected + + # assert that the local candidate's state was updated after the packet was discarded + assert [ + %{ + state: :succeeded, + packets_discarded_on_send: 1, + bytes_discarded_on_send: 8 + } + ] = Map.values(ice_agent.checklist) end test "relay connection" do From 9c2108ff706a920956a1f080bc98867b8276ac73 Mon Sep 17 00:00:00 2001 From: Karol Konkol Date: Sun, 29 Jun 2025 17:06:53 +0200 Subject: [PATCH 2/8] Update workflow - run one test --- .github/workflows/ci.yml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5a57845..00410eb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,7 +36,8 @@ jobs: otp-version: ${{matrix.otp}} elixir-version: ${{matrix.elixir}} - run: mix deps.get - - run: mix coveralls.json - - uses: codecov/codecov-action@v4 - with: - token: ${{ secrets.CODECOV_TOKEN }} \ No newline at end of file + - run: mix test test/integration/p2p_test.exs:10 + # - run: mix coveralls.json + # - uses: codecov/codecov-action@v4 + # with: + # token: ${{ secrets.CODECOV_TOKEN }} \ No newline at end of file From 73e1f92457373bf91300c2ecd909f5a56ee7c7e3 Mon Sep 17 00:00:00 2001 From: Karol Konkol Date: Mon, 30 Jun 2025 12:19:55 +0200 Subject: [PATCH 3/8] Close candidate pair on conn check send error --- lib/ex_ice/priv/ice_agent.ex | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index 8d00756..b746e94 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/lib/ex_ice/priv/ice_agent.ex @@ -3013,7 +3013,8 @@ defmodule ExICE.Priv.ICEAgent do pair = %CandidatePair{ pair | packets_discarded_on_send: pair.packets_discarded_on_send + 1, - bytes_discarded_on_send: pair.bytes_discarded_on_send + byte_size(raw_req) + bytes_discarded_on_send: pair.bytes_discarded_on_send + byte_size(raw_req), + state: :failed } put_in(ice_agent.checklist[pair.id], pair) From 3a8178631898fbadf72e4949a9bc1cd32390d2b8 Mon Sep 17 00:00:00 2001 From: Karol Konkol Date: Mon, 30 Jun 2025 12:22:25 +0200 Subject: [PATCH 4/8] Revert workflow changes --- .github/workflows/ci.yml | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 00410eb..5a57845 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,8 +36,7 @@ jobs: otp-version: ${{matrix.otp}} elixir-version: ${{matrix.elixir}} - run: mix deps.get - - run: mix test test/integration/p2p_test.exs:10 - # - run: mix coveralls.json - # - uses: codecov/codecov-action@v4 - # with: - # token: ${{ secrets.CODECOV_TOKEN }} \ No newline at end of file + - run: mix coveralls.json + - uses: codecov/codecov-action@v4 + with: + token: ${{ secrets.CODECOV_TOKEN }} \ No newline at end of file From 30102666356d18b95caff8e5435d778c63f220f5 Mon Sep 17 00:00:00 2001 From: Karol Konkol Date: Mon, 30 Jun 2025 13:31:04 +0200 Subject: [PATCH 5/8] Add conn check eperm test --- test/priv/ice_agent_test.exs | 46 ++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/test/priv/ice_agent_test.exs b/test/priv/ice_agent_test.exs index 9ac63d2..b2d0fda 100644 --- a/test/priv/ice_agent_test.exs +++ b/test/priv/ice_agent_test.exs @@ -1245,6 +1245,8 @@ defmodule ExICE.Priv.ICEAgentTest do end end + @conn_check_byte_size 92 + describe "connectivity check" do setup do ice_agent = @@ -1563,6 +1565,50 @@ defmodule ExICE.Priv.ICEAgentTest do assert ice_agent.state == :completed end + + test "candidate fails to send conn check" do + # 1. replace candidate with the mock one that always fails to send data + # 2. assert that after unsuccessful conn check sending, ice_agent move conn pair to the failed state + + ice_agent = + ICEAgent.new( + controlling_process: self(), + role: :controlling, + if_discovery_module: IfDiscovery.Mock, + transport_module: Transport.Mock + ) + |> ICEAgent.set_remote_credentials("someufrag", "somepwd") + |> ICEAgent.gather_candidates() + |> ICEAgent.add_remote_candidate(@remote_cand) + |> ICEAgent.end_of_candidates() + + assert ice_agent.gathering_state == :complete + + # replace candidate with the mock one + [local_cand] = Map.values(ice_agent.local_cands) + mock_cand = %Candidate.Mock{base: local_cand.base} + ice_agent = %{ice_agent | local_cands: %{mock_cand.base.id => mock_cand}} + + # try to send conn check + ice_agent = ICEAgent.handle_ta_timeout(ice_agent) + + assert ice_agent.state == :checking + + # assert that the candidate pair has moved to a failed state + # and that the state was updated after the packet was discarded + assert [ + %{ + state: :failed, + valid?: false, + packets_discarded_on_send: 1, + bytes_discarded_on_send: @conn_check_byte_size + } + ] = Map.values(ice_agent.checklist) + + ice_agent = ICEAgent.handle_ta_timeout(ice_agent) + + assert ice_agent.state == :failed + end end describe "connectivity check with aggressive nomination" do From d9bf7645ec7023d87a52499675f6534277f0354c Mon Sep 17 00:00:00 2001 From: Karol Konkol Date: Mon, 30 Jun 2025 17:45:53 +0200 Subject: [PATCH 6/8] Requested changes --- lib/ex_ice/priv/ice_agent.ex | 5 ++++- test/priv/ice_agent_test.exs | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index b746e94..0376e99 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/lib/ex_ice/priv/ice_agent.ex @@ -1744,6 +1744,7 @@ defmodule ExICE.Priv.ICEAgent do conn_check_pair = %CandidatePair{ conn_check_pair | state: :failed, + valid?: false, non_symmetric_responses_received: conn_check_pair.non_symmetric_responses_received + 1 } @@ -1817,6 +1818,7 @@ defmodule ExICE.Priv.ICEAgent do conn_check_pair = %CandidatePair{ conn_check_pair | state: :failed, + valid?: false, responses_received: conn_check_pair.responses_received + 1 } @@ -3014,7 +3016,8 @@ defmodule ExICE.Priv.ICEAgent do pair | packets_discarded_on_send: pair.packets_discarded_on_send + 1, bytes_discarded_on_send: pair.bytes_discarded_on_send + byte_size(raw_req), - state: :failed + state: :failed, + valid?: false } put_in(ice_agent.checklist[pair.id], pair) diff --git a/test/priv/ice_agent_test.exs b/test/priv/ice_agent_test.exs index b2d0fda..f79566a 100644 --- a/test/priv/ice_agent_test.exs +++ b/test/priv/ice_agent_test.exs @@ -1566,9 +1566,9 @@ defmodule ExICE.Priv.ICEAgentTest do assert ice_agent.state == :completed end - test "candidate fails to send conn check" do + test "failure on send" do # 1. replace candidate with the mock one that always fails to send data - # 2. assert that after unsuccessful conn check sending, ice_agent move conn pair to the failed state + # 2. assert that after unsuccessful conn check sending, ice_agent moves conn pair to the failed state ice_agent = ICEAgent.new( From fb8e5daaca3c1b012e666490c1f185ba67927af8 Mon Sep 17 00:00:00 2001 From: Karol Konkol Date: Tue, 1 Jul 2025 13:10:55 +0200 Subject: [PATCH 7/8] Add nomination failure test --- test/priv/ice_agent_test.exs | 63 +++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 8 deletions(-) diff --git a/test/priv/ice_agent_test.exs b/test/priv/ice_agent_test.exs index f79566a..7229977 100644 --- a/test/priv/ice_agent_test.exs +++ b/test/priv/ice_agent_test.exs @@ -303,7 +303,7 @@ defmodule ExICE.Priv.ICEAgentTest do ice_agent = put_in(ice_agent.local_cands[cand.base.id], cand) [pair] = Map.values(ice_agent.checklist) - pair = %{pair | state: :failed} + pair = %{pair | state: :failed, valid?: false} ice_agent = put_in(ice_agent.checklist[pair.id], pair) # try to feed data on closed candidate, it should be ignored @@ -363,7 +363,7 @@ defmodule ExICE.Priv.ICEAgentTest do assert ice_agent.state == :closed assert ice_agent.gathering_state == :complete - assert [%{state: :failed} = pair] = Map.values(ice_agent.checklist) + assert [%{state: :failed, valid?: false} = pair] = Map.values(ice_agent.checklist) assert [%{base: %{closed?: true}}] = Map.values(ice_agent.local_cands) # make sure that sockets and remote cands were not cleared assert [_remote_cand] = Map.values(ice_agent.remote_cands) @@ -454,7 +454,7 @@ defmodule ExICE.Priv.ICEAgentTest do # mark pair as failed [pair] = Map.values(ice_agent.checklist) - ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed}) + ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed, valid?: false}) # clear ta_timer, ignore outgoing binding request that has been generated ice_agent = ICEAgent.handle_ta_timeout(ice_agent) @@ -512,8 +512,7 @@ defmodule ExICE.Priv.ICEAgentTest do # mark pair as failed [pair] = Map.values(ice_agent.checklist) - ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed}) - + ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed, valid?: false}) # clear ta_timer, ignore outgoing binding request that has been generated ice_agent = ICEAgent.handle_ta_timeout(ice_agent) assert ice_agent.ta_timer == nil @@ -1246,6 +1245,7 @@ defmodule ExICE.Priv.ICEAgentTest do end @conn_check_byte_size 92 + @conn_check_with_nomination_byte_size 96 describe "connectivity check" do setup do @@ -1518,7 +1518,7 @@ defmodule ExICE.Priv.ICEAgentTest do resp ) - assert [%CandidatePair{state: :failed}] = Map.values(ice_agent.checklist) + assert [%CandidatePair{state: :failed, valid?: false}] = Map.values(ice_agent.checklist) assert [new_pair] = Map.values(ice_agent.checklist) assert new_pair.state == :failed assert new_pair.responses_received == pair.responses_received @@ -1609,6 +1609,53 @@ defmodule ExICE.Priv.ICEAgentTest do assert ice_agent.state == :failed end + + test "failure on send, when nominating" do + # 1. make ice agent connected + # 2. replace candidate with the mock one that always fails to send data + # 3. assert that after unsuccessful nomination sending, ice_agent moves conn pair to the failed state + + ice_agent = + ICEAgent.new( + controlling_process: self(), + role: :controlling, + if_discovery_module: IfDiscovery.Mock, + transport_module: Transport.Mock + ) + |> ICEAgent.set_remote_credentials("someufrag", "somepwd") + |> ICEAgent.gather_candidates() + |> ICEAgent.add_remote_candidate(@remote_cand) + + assert ice_agent.gathering_state == :complete + + # make ice_agent connected + ice_agent = connect(ice_agent) + + # replace candidate with the mock one + [local_cand] = Map.values(ice_agent.local_cands) + mock_cand = %Candidate.Mock{base: local_cand.base} + ice_agent = %{ice_agent | local_cands: %{mock_cand.base.id => mock_cand}} + + # trigger pair nomination + ice_agent = ICEAgent.end_of_candidates(ice_agent) + + # assert that the candidate pair has moved to a failed state + # and that the state was updated after the packet was discarded + assert [ + %{ + state: :failed, + valid?: false, + packets_discarded_on_send: 1, + bytes_discarded_on_send: @conn_check_with_nomination_byte_size + } + ] = Map.values(ice_agent.checklist) + + assert ice_agent.state == :connected + + ice_agent = ICEAgent.handle_ta_timeout(ice_agent) + + assert ice_agent.state == :failed + end end describe "connectivity check with aggressive nomination" do @@ -1989,7 +2036,7 @@ defmodule ExICE.Priv.ICEAgentTest do ice_agent = ICEAgent.handle_pair_timeout(ice_agent) # assert that the pair is marked as failed - assert [%CandidatePair{state: :failed}] = Map.values(ice_agent.checklist) + assert [%CandidatePair{state: :failed, valid?: false}] = Map.values(ice_agent.checklist) # trigger eoc timeout ice_agent = ICEAgent.handle_eoc_timeout(ice_agent) @@ -2019,7 +2066,7 @@ defmodule ExICE.Priv.ICEAgentTest do # mark pair as failed [pair] = Map.values(ice_agent.checklist) - ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed}) + ice_agent = put_in(ice_agent.checklist[pair.id], %{pair | state: :failed, valid?: false}) # set eoc flag failed_ice_agent = ICEAgent.end_of_candidates(ice_agent) From 13242198d55f04af810eead3b02ea61da2eaf2ca Mon Sep 17 00:00:00 2001 From: Karol Konkol Date: Mon, 7 Jul 2025 13:49:52 +0200 Subject: [PATCH 8/8] Update the connection state after all actions that may affect it --- lib/ex_ice/priv/ice_agent.ex | 9 +++++++-- test/priv/ice_agent_test.exs | 8 -------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/ex_ice/priv/ice_agent.ex b/lib/ex_ice/priv/ice_agent.ex index 0376e99..27ce8eb 100644 --- a/lib/ex_ice/priv/ice_agent.ex +++ b/lib/ex_ice/priv/ice_agent.ex @@ -485,7 +485,9 @@ defmodule ExICE.Priv.ICEAgent do Logger.debug("Setting end-of-candidates flag.") ice_agent = %{ice_agent | eoc: true} # check whether it's time to nominate and if yes, try noimnate - maybe_nominate(ice_agent) + ice_agent + |> maybe_nominate() + |> update_connection_state() end @spec send_data(t(), binary()) :: t() @@ -580,6 +582,7 @@ defmodule ExICE.Priv.ICEAgent do |> update_gathering_state() |> update_connection_state() |> maybe_nominate() + |> update_connection_state() if ice_agent.state in [:completed, :failed] do update_ta_timer(ice_agent) @@ -591,7 +594,9 @@ defmodule ExICE.Priv.ICEAgent do ice_agent {type, tr} -> - execute_transaction(ice_agent, type, tr) + ice_agent + |> execute_transaction(type, tr) + |> update_connection_state() end # schedule next check and call update_ta_timer diff --git a/test/priv/ice_agent_test.exs b/test/priv/ice_agent_test.exs index 7229977..b250401 100644 --- a/test/priv/ice_agent_test.exs +++ b/test/priv/ice_agent_test.exs @@ -1592,8 +1592,6 @@ defmodule ExICE.Priv.ICEAgentTest do # try to send conn check ice_agent = ICEAgent.handle_ta_timeout(ice_agent) - assert ice_agent.state == :checking - # assert that the candidate pair has moved to a failed state # and that the state was updated after the packet was discarded assert [ @@ -1605,8 +1603,6 @@ defmodule ExICE.Priv.ICEAgentTest do } ] = Map.values(ice_agent.checklist) - ice_agent = ICEAgent.handle_ta_timeout(ice_agent) - assert ice_agent.state == :failed end @@ -1650,10 +1646,6 @@ defmodule ExICE.Priv.ICEAgentTest do } ] = Map.values(ice_agent.checklist) - assert ice_agent.state == :connected - - ice_agent = ICEAgent.handle_ta_timeout(ice_agent) - assert ice_agent.state == :failed end end