T3871: handle phase-2 rename failures in safe_bulk_rename()

The phase-2 (scratch -> final target) loop ignored rename_interface()'s
return value entirely: applied[old] was already set in phase 1 (staging
to the scratch name), so a failed final rename still got reported as a
success, and the unconditional `ip link set dev {target} up` tried to
bring up a name that was never actually assigned - the interface stays
down, stuck under its scratch (vyethN) name, with nothing tracking it
correctly. Downstream, sync_rescan_hints() would see it under that
scratch name and (since its MAC is normally a configured one, that's
the whole reason it was being renamed) skip leaving a rescan hint for
it too, since a configured MAC never gets a hint regardless of what
name it's stuck under - so the interface would just silently vanish
from the admin's view.

Track (old, target) per scratch entry instead of target alone, so a
phase-2 failure can remove the corresponding old->target entry from
applied and bring the interface back up under its actual (scratch)
name instead of the target it never reached. Successful renames are
unaffected, and one failure in a batch doesn't affect the others.

Adds TestSafeBulkRename, the first direct unit test for this function:
the success path, a phase-2 failure not being reported as applied, the
interface being brought back up under its scratch name rather than the
unassigned target, and one failure not affecting other renames in the
same batch. Verified all three failure-mode tests actually fail
against the original code before confirming they pass with the fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: c-po

src/system/vyos-net-name-resolve.py

@@ -356,12 +356,17 @@ def safe_bulk_rename(plan: dict) -> dict:
     for old, target in plan.items():
         tmp = f'vyeth{get_ifindex(old)}'
         if rename_interface(old, tmp):
-            scratch[tmp] = target
+            scratch[tmp] = (old, target)
             applied[old] = target
 
-    for tmp, target in scratch.items():
-        rename_interface(tmp, target)
-        run(f'ip link set dev {target} up')
+    for tmp, (old, target) in scratch.items():
+        if rename_interface(tmp, target):
+            run(f'ip link set dev {target} up')
+        else:
+            # still sitting under the scratch name, not the target - don't
+            # report it as renamed, and don't leave it down indefinitely
+            del applied[old]
+            run(f'ip link set dev {tmp} up')
 
     return applied
 

src/tests/test_net_name_resolve.py

@@ -329,6 +329,60 @@ def test_bootstrap_targets_never_collide_with_existing_plan_values(self):
         self.assertNotEqual(plan.get('newnic'), 'eth0')
 
 
+class TestSafeBulkRename(unittest.TestCase):
+    """The two-phase rename must accurately report what actually happened -
+    a phase-2 (scratch -> final target) failure must not be reported as a
+    successful rename, and must not leave the interface down indefinitely
+    under a name nothing else knows about.
+    """
+
+    def setUp(self):
+        patcher = mock.patch.object(resolver, 'get_ifindex',
+                                     side_effect=lambda name: name)
+        patcher.start()
+        self.addCleanup(patcher.stop)
+
+    def test_all_renames_succeed(self):
+        with mock.patch.object(resolver, 'run', return_value=0):
+            applied = resolver.safe_bulk_rename({'eth5': 'eth0'})
+        self.assertEqual(applied, {'eth5': 'eth0'})
+
+    def test_phase_two_failure_is_not_reported_as_applied(self):
+        # 'ip link set dev vyetheth5 name eth0' fails (exit 1); everything
+        # else (down, the phase-1 rename to scratch) succeeds
+        def fake_run(command, *_a, **_kw):
+            return 1 if command == 'ip link set dev vyetheth5 name eth0' else 0
+
+        with mock.patch.object(resolver, 'run', side_effect=fake_run):
+            applied = resolver.safe_bulk_rename({'eth5': 'eth0'})
+
+        self.assertNotIn('eth5', applied)
+        self.assertNotEqual(applied.get('eth5'), 'eth0')
+
+    def test_phase_two_failure_brings_scratch_name_back_up(self):
+        calls = []
+
+        def fake_run(command, *_a, **_kw):
+            calls.append(command)
+            return 1 if command == 'ip link set dev vyetheth5 name eth0' else 0
+
+        with mock.patch.object(resolver, 'run', side_effect=fake_run):
+            resolver.safe_bulk_rename({'eth5': 'eth0'})
+
+        self.assertIn('ip link set dev vyetheth5 up', calls)
+        self.assertNotIn('ip link set dev eth0 up', calls)
+
+    def test_one_failure_does_not_affect_other_renames_in_the_batch(self):
+        def fake_run(command, *_a, **_kw):
+            return 1 if command == 'ip link set dev vyetheth5 name eth0' else 0
+
+        with mock.patch.object(resolver, 'run', side_effect=fake_run):
+            applied = resolver.safe_bulk_rename({'eth5': 'eth0', 'eth9': 'eth1'})
+
+        self.assertNotIn('eth5', applied)
+        self.assertEqual(applied.get('eth9'), 'eth1')
+
+
 class TestGetPermanentMac(unittest.TestCase):
     """Part of the original race is reading a MAC before the driver has
     programmed the permanent address. Verify the ethtool-first,