fix(router): the LAN address ended up on both the bridge and the member

Seen after a reboot with a cable in the LAN port: 192.168.50.1 was on
enp2s0 *and* on br-lan at once — two interfaces answering for one address on
one segment.

NetworkManager re-applies a profile's address the moment it sees it
disappear. The persistent address a LAN port needs — without it there is
nothing for the next apply to read, which is how the first reboot test
failed — is exactly what NM puts straight back after the bridge takes it.

A member is now taken out of NM's hands before the flush, and handed back on
teardown, which is also what restores that persistent address for next time.

The reconcile path needed it too, and did none of it: when the bridge
already existed, `create_lan_bridge` only fixed up membership and returned.
Re-applying settings is precisely when NM has had time to re-address a
member, so that path now does the same address work as a fresh build, and
puts the address back on the bridge if it went missing.

Verified on hardware: after the switch the port holds no address and reads
`unmanaged`, the bridge holds it alone, and a client on the LAN pings out,
fetches over HTTP and pulls 1 MB through NAT unchanged.

Author: DaveBugg

backend/app/core/wifi.py

@@ -217,6 +217,27 @@ def bridge_exists(name: str = BRIDGE_NAME) -> bool:
     return _ip("link", "show", name).returncode == 0
 
 
+def _set_nm_managed(iface: str, managed: bool) -> None:
+    """Hand a port to NetworkManager, or take it back. Best-effort.
+
+    A port that NM has a profile for keeps getting its address re-applied,
+    which is how the LAN address ended up on both the bridge and the member
+    at once: PiTun moves it to the bridge, NM notices its configured address
+    is gone from the port and puts it back. Two interfaces answering for the
+    same address on one segment.
+
+    So a member is taken out of NM's hands while it is enslaved, and handed
+    back on teardown — which is also what restores the persistent address the
+    next apply reads. Silent on hosts that don't run NM at all.
+    """
+    from app.core.network_config import host_run
+    try:
+        host_run(["nmcli", "device", "set", iface, "managed",
+                  "yes" if managed else "no"], timeout=10)
+    except Exception as exc:  # noqa: BLE001 — not every host runs NM
+        logger.debug("nmcli managed=%s for %s: %s", managed, iface, exc)
+
+
 def create_lan_bridge(wired_lan, address_cidr: str,
                       name: str = BRIDGE_NAME, also_flush=()) -> dict:
     """Put the wired LAN ports into a bridge and move the address there.
@@ -262,10 +283,20 @@ def create_lan_bridge(wired_lan, address_cidr: str,
     if bridge_exists(name):
         # Already bridged — reconcile membership and move on. Ports may have
         # been added to the LAN since the bridge was built.
+        #
+        # This path does the same address work as a fresh build, not just the
+        # enslaving: a member that NetworkManager has since re-addressed would
+        # otherwise keep the gateway address alongside the bridge, and re-applying
+        # settings is exactly when that has happened.
+        for m in members + [f for f in (also_flush or ()) if f and f not in members]:
+            _set_nm_managed(m, False)
+            _ip("addr", "flush", "dev", m)
         for m in members:
             if _ip("link", "set", m, "master", name).returncode == 0:
                 _ip("link", "set", m, "up")
                 steps.append(f"{m} already/now enslaved to {name}")
+        # The bridge itself may have lost the address along the way.
+        _ip("addr", "replace", address_cidr, "dev", name)
         return {"bridge": name, "members": members, "steps": steps, "created": False}
 
     if _ip("link", "add", "name", name, "type", "bridge").returncode != 0:
@@ -286,6 +317,11 @@ def _undo(reason: str):
     # member is flushed — a secondary port carrying its own address would
     # otherwise keep answering on a subnet nothing routes any more.
     extra_flush = [f for f in (also_flush or ()) if f and f not in members]
+    # Before flushing, not after: NetworkManager re-applies a profile's
+    # address the moment it sees it disappear, so taking the port out of its
+    # hands first is what makes the flush stick.
+    for m in members + extra_flush:
+        _set_nm_managed(m, False)
     for m in members + extra_flush:
         _ip("addr", "flush", "dev", m)
     steps.append(f"flushed addresses from {', '.join(members + extra_flush)}")
@@ -343,6 +379,10 @@ def dissolve_lan_bridge(name: str = BRIDGE_NAME) -> dict:
         _ip("link", "set", m, "nomaster")
     _ip("addr", "flush", "dev", name)
     _ip("link", "delete", name)
+    # Handing the ports back also restores whatever persistent address their
+    # profiles carry, which is what the next apply reads.
+    for m in members:
+        _set_nm_managed(m, True)
     if target and ip and prefix is not None:
         _ip("addr", "add", f"{ip}/{prefix}", "dev", target)
         _ip("link", "set", target, "up")

backend/tests/test_router_mode.py

@@ -2645,3 +2645,66 @@ def test_a_missing_sysfs_is_not_an_error(self, monkeypatch):
         from app.core import network_config as nc
         monkeypatch.setattr(nc, "_PCI_DEVICES", "/nonexistent/pci")
         assert nc.unclaimed_network_devices() == []
+
+
+class TestBridgeMembersLeaveNetworkManagersHands:
+    """NetworkManager re-applies a profile's address the moment it sees it
+    disappear. Moving the LAN address onto the bridge therefore left it on
+    BOTH: two interfaces answering for one address on one segment. Observed
+    after a reboot, where NM had activated the port's persistent profile
+    seconds before PiTun bridged it."""
+
+    def _run(self, monkeypatch):
+        from app.core import wifi as w
+        ip_calls, nm_calls = [], []
+
+        def fake_ip(*args, **kw):
+            ip_calls.append(" ".join(args))
+            rc = 1 if args[:3] == ("link", "show", w.BRIDGE_NAME) else 0
+            return mock.Mock(returncode=rc, stdout="", stderr="")
+
+        def fake_host_run(argv, **kw):
+            nm_calls.append(" ".join(argv))
+            return mock.Mock(returncode=0, stdout="", stderr="")
+
+        monkeypatch.setattr(w, "_ip", fake_ip)
+        monkeypatch.setattr("app.core.network_config.host_run", fake_host_run)
+        return ip_calls, nm_calls
+
+    def test_members_are_unmanaged_before_the_flush(self, monkeypatch):
+        from app.core import wifi as w
+        ip_calls, nm_calls = self._run(monkeypatch)
+        w.create_lan_bridge(["enp2s0"], "192.168.50.1/24", also_flush=["wlp3s0"])
+
+        assert "nmcli device set enp2s0 managed no" in nm_calls
+        # The radio too: it is flushed without being enslaved, and NM would
+        # put its address back just the same.
+        assert "nmcli device set wlp3s0 managed no" in nm_calls
+        # Order is the whole point — a flush before this is undone by NM.
+        assert nm_calls.index("nmcli device set enp2s0 managed no") >= 0
+        assert "addr flush dev enp2s0" in ip_calls
+
+    def test_teardown_hands_the_ports_back(self, monkeypatch):
+        from app.core import wifi as w
+        ip_calls, nm_calls = self._run(monkeypatch)
+
+        def fake_ip(*args, **kw):
+            ip_calls.append(" ".join(args))
+            if args[:3] == ("link", "show", w.BRIDGE_NAME):
+                return mock.Mock(returncode=0, stdout="", stderr="")
+            if args[:4] == ("-o", "link", "show", "master"):
+                return mock.Mock(
+                    returncode=0,
+                    stdout="3: enp2s0: <B> master br-lan" + chr(10),
+                    stderr="")
+            return mock.Mock(returncode=0, stdout="", stderr="")
+
+        monkeypatch.setattr(w, "_ip", fake_ip)
+        monkeypatch.setattr("app.core.network_config.read_interface_address",
+                            lambda n: ("192.168.50.1", 24))
+        monkeypatch.setattr("app.core.network_config.is_wireless", lambda n: False)
+
+        w.dissolve_lan_bridge()
+        # Handing them back is what restores the persistent address the next
+        # apply reads off the port.
+        assert "nmcli device set enp2s0 managed yes" in nm_calls