[fix] remove ASIC ports before replication cleanup on empty transition (#232)

When all members are removed from a multicast group, the empty transition path cleaned up replication table entries but skipped process_membership_changes, leaving stale ports in the ASIC groups. Subsequent re-adds failed with "already contains port" (500).

This fix calls process_membership_changes before cleanup_empty_group_replication so mc_port_remove runs on every port.

Author: zeeshanlakhani

dpd-client/tests/integration_tests/mcast.rs

@@ -4762,7 +4762,11 @@ async fn test_multicast_empty_then_add_members_ipv6() -> TestResult {
     // Update the internal group to add members (2 external, 1 underlay)
     // Meaning: two decap/port-bitmap members.
     let update_entry = types::MulticastGroupUpdateUnderlayEntry {
-        members: vec![external_member1, external_member2, underlay_member],
+        members: vec![
+            external_member1.clone(),
+            external_member2.clone(),
+            underlay_member.clone(),
+        ],
     };
 
     let ipv6_update = types::UnderlayMulticastIpv6(match internal_group_ip {
@@ -4945,6 +4949,43 @@ async fn test_multicast_empty_then_add_members_ipv6() -> TestResult {
 
     switch.packet_test(vec![send_final], expected_final)?;
 
+    // Re-add members after removing all. This exercises the full
+    // empty -> members -> empty -> members cycle and verifies that
+    // ASIC port state is properly cleaned up during the transition
+    // to empty, so that re-adding does not fail with a duplicate
+    // port error.
+    let readd_entry = types::MulticastGroupUpdateUnderlayEntry {
+        members: vec![external_member1, external_member2, underlay_member],
+    };
+
+    switch
+        .client
+        .multicast_group_update_underlay(
+            &ipv6_update,
+            &make_tag(TEST_TAG),
+            &readd_entry,
+        )
+        .await
+        .expect("Should re-add members after removing all");
+
+    let groups_after_readd = switch
+        .client
+        .multicast_groups_list_stream(None)
+        .try_collect::<Vec<_>>()
+        .await
+        .expect("Should list groups after re-add");
+
+    let readd_internal = groups_after_readd
+        .iter()
+        .find(|g| get_group_ip(g) == internal_group_ip)
+        .expect("Should find the internal group");
+
+    assert_eq!(
+        get_members(readd_internal).map(|m| m.len()).unwrap_or(0),
+        3,
+        "Internal group should have 3 members after re-add"
+    );
+
     cleanup_test_group(&switch, external_group_ip, TEST_TAG).await.unwrap();
     cleanup_test_group(&switch, internal_group_ip, TEST_TAG).await
 }

dpd/src/mcast/mod.rs

@@ -829,25 +829,41 @@ pub(crate) fn modify_group_internal(
     // Configure replication based on member count transitions
     let replication_info = match (
         new_group_info.members.is_empty(),
-        group_entry.replication_info.is_some(),
+        &group_entry.replication_info,
     ) {
-        (true, true) => {
-            // Transition from members to empty - cleanup tables
+        (true, Some(repl_info)) => {
+            // Transition from members to empty.
+            //
+            // First, remove ports from ASIC groups before cleaning up
+            // replication table entries, otherwise stale ports cause subsequent
+            // re-adds to fail with "already contains port".
+            process_membership_changes(
+                s,
+                group_ip.into(),
+                &new_group_info.members,
+                &group_entry,
+                repl_info,
+            )
+            .inspect_err(|_e| {
+                mcast.groups.insert(group_ip.into(), group_entry.clone());
+            })
+            .map_err(|e| rollback_ctx.rollback_and_restore(e))?;
+
             cleanup_empty_group_replication(s, group_ip.into(), &group_entry)
                 .map_err(|e| rollback_ctx.rollback_and_restore(e))?;
-            // Immediately clear replication_info to maintain consistency
+
             group_entry.replication_info = None;
             None
         }
-        (false, false) => {
+        (false, None) => {
             // Transition from empty to members - configure replication
             Some(configure_replication(group_entry.external_group_id()))
         }
-        (false, true) => {
+        (false, Some(_)) => {
             // Already has members and replication - keep existing
             group_entry.replication_info.clone()
         }
-        (true, false) => {
+        (true, None) => {
             // Already empty and no replication - keep none
             None
         }
@@ -870,7 +886,7 @@ pub(crate) fn modify_group_internal(
         s,
         group_ip.into(),
         &new_group_info.members,
-        &mut group_entry,
+        &group_entry,
         repl_info,
     )
     .inspect_err(|_e| {
@@ -1489,7 +1505,7 @@ fn process_membership_changes(
     s: &Switch,
     group_ip: IpAddr,
     new_members: &[MulticastGroupMember],
-    group_entry: &mut MulticastGroup,
+    group_entry: &MulticastGroup,
     replication_info: &MulticastReplicationInfo,
 ) -> DpdResult<(Vec<MulticastGroupMember>, Vec<MulticastGroupMember>)> {
     // First validate that IPv4 doesn't have underlay members