diff --git a/meshterm/core/connection.py b/meshterm/core/connection.py index 3362559..d55f1d0 100644 --- a/meshterm/core/connection.py +++ b/meshterm/core/connection.py @@ -174,6 +174,11 @@ class ContactNotOnDeviceError(DeviceCommandError): for it (see :mod:`meshterm.core.contact_store`): a contact the firmware has since dropped still lists, and only the send finds out it is gone. + :meth:`Device.remove_contact` raises it for the same lookup against the same table, + where it means the opposite thing: nothing left to delete on the radio. A removal + therefore *reports* it and carries on dropping the contact from what MeshTerm + remembers, rather than failing and leaving a row the reader cannot get rid of. + Attributes: contact: The recipient the device could not find. """ @@ -535,8 +540,11 @@ class Device(ABC): node: The contact to remove. Raises: + ContactNotOnDeviceError: If the device holds no contact with that key — which + is not a failed removal but a removal with nothing left to do, so callers + drop it from what MeshTerm remembers and say so. DeviceCommandError: If the contact carries no public key to address it by, or - the device rejected the removal. + the device rejected the removal for any other reason. """ @abstractmethod @@ -1742,7 +1750,18 @@ class MeshCoreDevice(Device): async def remove_contact(self, node: Contact) -> None: # noqa: D102 - inherited docstring mc = self._require() pub = self._node_pubkey(node) # raises DeviceCommandError if it has no key - self._ok(await mc.commands.remove_contact(pub)) + result = await mc.commands.remove_contact(pub) + if result is not None and getattr(result, "is_error", lambda: False)(): + # A contact the firmware doesn't hold is the one rejection that isn't a + # failure: the list a screen removes from is the union of this table and the + # contacts MeshTerm remembers for the device, so the entry the reader is + # deleting may only ever have existed on our side. Same class the send path + # raises, for the same reason — the caller can finish the job. + if error_code(result) == _ERR_NOT_FOUND: + raise ContactNotOnDeviceError(node) + raise DeviceCommandError( + f"couldn't remove {node.name} from the device: {reject_reason(result)}" + ) async def get_tx_power(self) -> Optional[int]: # noqa: D102 - inherited docstring info = await self.get_self_info() @@ -2736,6 +2755,11 @@ class MockDevice(Device): async def remove_contact(self, node: Contact) -> None: # noqa: D102 - inherited docstring await asyncio.sleep(0) key = self._mock_key(node) + # Firmware can only delete a contact it holds, and answers ERR_CODE_NOT_FOUND for + # one it doesn't; the simulator refuses the same way so the "wasn't on the device, + # removed here anyway" path is walkable without a radio. + if key not in {self._mock_key(c) for c in self._contacts}: + raise ContactNotOnDeviceError(node) self._contacts = [c for c in self._contacts if self._mock_key(c) != key] async def get_tx_power(self) -> Optional[int]: # noqa: D102 - inherited docstring diff --git a/meshterm/ui/contacts_screen.py b/meshterm/ui/contacts_screen.py index 66966c5..5e5a20d 100644 --- a/meshterm/ui/contacts_screen.py +++ b/meshterm/ui/contacts_screen.py @@ -32,6 +32,7 @@ from typing import TYPE_CHECKING from rich.text import Text +from ..core.connection import ContactNotOnDeviceError from .contactlist import ContactListScreen, ContactRow from .menus import marked_label, menu_rows from .tui import Choice, Separator @@ -321,6 +322,12 @@ async def _purge_stale(ctx: "AppContext", self_key: str) -> int: for contact in victims: try: await device.remove_contact(contact) + except ContactNotOnDeviceError: + # The device has no such contact, so the sweep's work for it is already + # done: this one is listed only because MeshTerm remembers it for the + # device. It still counts as purged — forgetting it below is what makes + # the row go away — and there is nothing here for the reader to fix. + ctx.log.debug("contacts: %s was not on the device; purging ours", contact.name) except Exception as exc: # noqa: BLE001 - one bad removal shouldn't abort the sweep failed += 1 ctx.log.debug("contacts: purge failed for %s: %s", contact.name, exc) diff --git a/meshterm/ui/node_detail_screen.py b/meshterm/ui/node_detail_screen.py index 9f11371..dd69c5a 100644 --- a/meshterm/ui/node_detail_screen.py +++ b/meshterm/ui/node_detail_screen.py @@ -82,6 +82,7 @@ from typing import TYPE_CHECKING, Optional from rich.cells import cell_len from rich.text import Text +from ..core.connection import ContactNotOnDeviceError from ..core.geo import EARTH_RADIUS_KM, usable_fix from ..core.models import NODE_TYPE_LABELS, Contact, utcnow from ..platforms import Platform, on_platform @@ -1454,7 +1455,9 @@ async def _remove_contact( Returns: ``True`` if the contact was removed, ``False`` if the user cancelled or the device refused (the refusal is shown, never swallowed — a contact still on the radio must - not vanish from the list). + not vanish from the list). A device that has no such contact is not a refusal: the + removal finishes on our side and says so + (:class:`~meshterm.core.connection.ContactNotOnDeviceError`). """ from .surface import TuiUi @@ -1474,6 +1477,21 @@ async def _remove_contact( device = await ctx.device() try: await device.remove_contact(contact) + except ContactNotOnDeviceError: + # Nothing to delete on the radio — the entry only ever existed in what MeshTerm + # remembers for this device (or the firmware dropped it since we read the table). + # That is not a failed removal: the contact still goes, and the reader is told why + # the device had no part in it, because "removed" would otherwise be a claim about + # a radio that never held it. + ctx.log.debug("contacts: %s was not on the device; removing ours", contact.name) + await session.message_dialog( + Text( + f"⚠ {label} wasn't in this device's contacts — removed from the ones " + "MeshTerm remembers for it.", + style="warn", + ), + title="Remove contact", + ) except Exception as exc: # noqa: BLE001 - a refused removal is reported, not raised ctx.log.debug("contacts: remove failed for %s: %s", contact.name, exc) await session.message_dialog( diff --git a/tests/test_device_config.py b/tests/test_device_config.py index 8ed4c52..05f192a 100644 --- a/tests/test_device_config.py +++ b/tests/test_device_config.py @@ -557,6 +557,61 @@ async def test_a_refused_removal_is_shown_and_the_contact_stays() -> None: assert forgotten == [] # the device still holds it, so neither do we forget it +async def test_a_contact_the_device_never_held_is_still_removed_here() -> None: + """"Not on the device" is not a refusal — the removal finishes on our side, and says so. + + The list a screen removes from is the union of the device's table and the contacts + MeshTerm remembers for it, so the entry may only ever have existed on our side (or the + firmware dropped it since we read). Failing there left a row the reader could not get + rid of. Instead the reader is told the device had no part in it, our half is forgotten, + and the visit ends on the contact like any other removal. + """ + import asyncio + import logging + from types import SimpleNamespace + + from meshterm.core.connection import ContactNotOnDeviceError + from meshterm.core.models import Contact + from meshterm.ui.node_detail_screen import _remove_contact + from meshterm.ui.surface import TuiUi + from meshterm.ui.tui.session import TuiSession + + hub = Contact(name="Hub", public_key="3d" * 32, key_prefix="3d" * 6) + forgotten: list[tuple[str, str]] = [] + invalidated: list[bool] = [] + + class _Device: + async def remove_contact(self, contact) -> None: # noqa: ANN001 + await asyncio.sleep(0) + raise ContactNotOnDeviceError(contact) + + session = TuiSession() + ui = TuiUi(session) + + async def _accepted(*a, **k): # noqa: ANN001, ANN002, ANN003 + return True + + ui.dialog = _accepted # type: ignore[method-assign] + ctx = SimpleNamespace( + ui=ui, + log=logging.getLogger("test.remove"), + contact_store=SimpleNamespace( + forget=lambda dev, key: forgotten.append((dev, key)) + ), + devstate=SimpleNamespace(invalidate_contacts=lambda: invalidated.append(True)), + device=lambda: _device(_Device()), + ) + + task = asyncio.ensure_future(_remove_contact(ctx, hub, "cc" * 32, "Hub")) + told = await _step_until_screen(session, lambda s: "wasn't in this device" in _screen_text(s)) + text = _screen_text(told) + assert "Hub" in text and "⚠" in text # a warning, not the ✗ of a failed command + told.handle("escape") + assert await task is True + assert forgotten == [("cc" * 32, "3d" * 32)] + assert invalidated == [True] + + async def _true() -> bool: return True diff --git a/tests/test_skeleton.py b/tests/test_skeleton.py index f6c594d..08ac1ab 100644 --- a/tests/test_skeleton.py +++ b/tests/test_skeleton.py @@ -320,6 +320,8 @@ async def test_mock_device_honors_forced_path() -> None: async def test_mock_device_remove_contact_drops_it_from_the_table() -> None: """Removing a contact deletes it from the device's table (matched by public key).""" + from meshterm.core.connection import ContactNotOnDeviceError + device = MockDevice() await device.connect() before = await device.get_contacts() @@ -328,8 +330,10 @@ async def test_mock_device_remove_contact_drops_it_from_the_table() -> None: after = await device.get_contacts() assert "Alice" not in {c.name for c in after} assert len(after) == len(before) - 1 - # Removing one already gone is a no-op, not an error. - await device.remove_contact(alice) + # Removing one it no longer holds is refused the way firmware refuses it, so the + # screen's "wasn't on the device, removed here anyway" path is walkable on --mock. + with pytest.raises(ContactNotOnDeviceError): + await device.remove_contact(alice) assert len(await device.get_contacts()) == len(after)