From a65ce0ccf7a901e331d1020ca8fd9d9e688b4c75 Mon Sep 17 00:00:00 2001 From: Jean-Pierre Martineau Date: Fri, 10 Jul 2026 22:16:36 -0400 Subject: [PATCH] Exit in bounded time, connect BLE first try: teardown and startup hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two long-standing flakes, one root cause each: Quit with a serial companion often froze until the exit watchdog killed the process. The meshcore dispatcher's stop() awaits queue.join(), but its processor task exits after draining at most one event once stopped — so a routine advert/RX-log burst (two+ events queued at that instant) deadlocks the join forever (repro confirmed against the installed library). MeshCoreDevice.disconnect now gives the graceful teardown two seconds, then abandons it: the dispatcher task is cancelled synchronously and the raw transport closed directly, so the port is still released and even the forced path finishes well under the watchdog. BLE startup often failed until the user re-selected the device. Connecting by bare address makes bleak re-discover the peripheral with a fresh internal scan, which on Windows intermittently misses a slow-advertising companion. The discovery scan's live BLEDevice now rides along (DiscoveredDevice -> probe/context -> create_ble) so the connect opens the peripheral directly, and a transport-level ConnectionError is retried once before surfacing — the manual re-select was never anything but a retry. PIN/bond rejections still surface on the first attempt, unlooped. Verified against hardware: BLE info read off the Waymarker connects and exits cleanly. Co-Authored-By: Claude Fable 5 --- meshterm/context.py | 17 ++++- meshterm/core/connection.py | 146 ++++++++++++++++++++++++++++++++---- meshterm/core/discovery.py | 9 +++ tests/test_disconnect.py | 132 ++++++++++++++++++++++++++++++++ tests/test_discovery.py | 4 + 5 files changed, 294 insertions(+), 14 deletions(-) diff --git a/meshterm/context.py b/meshterm/context.py index b26e7d2..40b6ed8 100644 --- a/meshterm/context.py +++ b/meshterm/context.py @@ -264,8 +264,23 @@ class AppContext: # resolution, and no re-scan needed to reconnect to a known address. ble_address, ble_pin = self._resolve_ble_endpoint() if ble_address: + # When this session's scan discovered the device, hand its live BLEDevice to the + # connection so bleak opens it directly instead of re-discovering the address (a + # fresh internal scan that intermittently misses a slow-advertising companion). + # A remembered/explicit address has no scan result and connects by address alone. + selected = self.selected_device + ble_device = ( + selected.ble_device + if selected is not None and selected.is_ble and selected.address == ble_address + else None + ) self._device = make_device( - mock=False, port=None, transport="ble", address=ble_address, pin=ble_pin + mock=False, + port=None, + transport="ble", + address=ble_address, + pin=ble_pin, + ble_device=ble_device, ) self._active_transport = "ble" self._active_address = ble_address diff --git a/meshterm/core/connection.py b/meshterm/core/connection.py index 7757850..e4d704b 100644 --- a/meshterm/core/connection.py +++ b/meshterm/core/connection.py @@ -62,6 +62,29 @@ _MESSAGE_POLL_INTERVAL_S = 3.0 #: drain loop (seconds). _MESSAGE_GET_TIMEOUT_S = 5.0 +#: How long a graceful ``meshcore`` client teardown may take before it is abandoned and the +#: transport is force-closed instead (seconds). A healthy disconnect completes in well under a +#: second; the bound exists because the library's dispatcher shutdown can deadlock — its +#: ``queue.join()`` never returns when two or more events (a routine serial RX burst) are +#: queued at the moment of stop, since the processor task exits after draining only one. Kept +#: comfortably under the interactive session's 5-second exit watchdog so even the forced path +#: finishes as a *clean* exit rather than an ``os._exit`` reap. +_DISCONNECT_TIMEOUT_S = 2.0 + +#: Bound on the forced transport close that follows an abandoned graceful teardown (seconds). +#: ``_DISCONNECT_TIMEOUT_S + _FORCE_DISCONNECT_TIMEOUT_S`` stays under the exit watchdog. +_FORCE_DISCONNECT_TIMEOUT_S = 1.5 + +#: Total attempts at opening the BLE link before its failure is surfaced. Opening a BLE +#: connection on Windows is intermittently flaky (a slow-advertising peripheral is missed by +#: bleak's internal lookup, or the link-layer connect races the just-finished discovery scan); +#: a single retry recovers the common case without meaningfully delaying a genuinely absent +#: device. This retries only the *link open* — never a rejected PIN and never a mesh transmit. +_BLE_CONNECT_ATTEMPTS = 2 + +#: Pause between BLE link-open attempts (seconds), giving the OS radio a beat to settle. +_BLE_CONNECT_RETRY_DELAY_S = 1.0 + TX_POWER_MIN = 1 TX_POWER_MAX = 22 @@ -691,6 +714,7 @@ class MeshCoreDevice(Device): transport: str = "serial", address: Optional[str] = None, pin: Optional[str] = None, + ble_device: Optional[object] = None, ) -> None: """Initialize the device wrapper. @@ -705,6 +729,12 @@ class MeshCoreDevice(Device): transport: ``"serial"`` (default) or ``"ble"``. address: Bluetooth address (e.g. ``AA:BB:CC:DD:EE:FF``); BLE transport only. pin: Optional BLE pairing PIN, when the peripheral requires one (BLE only). + ble_device: The ``bleak.BLEDevice`` the discovery scan already found at + ``address``, when available (BLE only). Passing it lets the connect open the + peripheral directly instead of re-discovering it by address — on Windows a + bare-address connect runs a fresh internal scan that intermittently misses a + slow-advertising companion, which is the main source of flaky BLE startups. + Typed ``object`` so ``bleak`` need not be imported on non-BLE paths. """ self._port = port self._baudrate = baudrate @@ -712,6 +742,7 @@ class MeshCoreDevice(Device): self._transport = transport self._address = address self._pin = pin + self._ble_device = ble_device self._mc = None # type: ignore[var-annotated] # meshcore.MeshCore # Serializes channel reads. The meshcore library's get_channel waits for "the next # CHANNEL_INFO event" with no correlation to the index it asked for, and the dispatcher @@ -807,12 +838,7 @@ class MeshCoreDevice(Device): that was rejected. """ try: - return await mesh_core.create_ble( - address=self._address, - pin=self._pin, - default_timeout=self._connect_timeout, - auto_reconnect=False, - ) + return await self._create_ble_with_retry(mesh_core) except ImportError as exc: raise DeviceCommandError( "Bluetooth support requires the 'bleak' package, which isn't installed. " @@ -836,6 +862,57 @@ class MeshCoreDevice(Device): return await self._open_ble(mesh_core, allow_repair=False) raise DeviceAuthenticationError(self._ble_auth_message()) from exc + async def _create_ble_with_retry(self, mesh_core): # type: ignore[no-untyped-def] + """Call ``create_ble``, retrying the transport-level failures that are transient. + + The meshcore client raises a bare ``ConnectionError`` when the *link itself* could + not be opened — the peripheral wasn't found during bleak's internal lookup, or the + link-layer connect timed out. On Windows both are routinely transient: a companion + advertising on a slow interval is easily missed by a single scan window, and a + connect attempted right after the discovery scan can race the radio. Users learned + to work around it by re-selecting the device, which is nothing but a manual retry — + so retry here, briefly, before surfacing the failure. The already-discovered + ``BLEDevice`` (when the picker's scan produced one) is passed through so the client + connects to it directly instead of re-discovering the address. + + Only ``ConnectionError`` is retried: a PIN/bond rejection or any other GATT failure + propagates unchanged on the first attempt so the auth handling in :meth:`_open_ble` + (and a genuine wrong-PIN) is never looped. + + Args: + mesh_core: The imported ``meshcore.MeshCore`` class. + + Returns: + The connected ``MeshCore`` client, or ``None`` if the transport connected but + the peripheral never answered the identity handshake. + + Raises: + ConnectionError: If every attempt failed to open the link. + """ + last_exc: Optional[ConnectionError] = None + for attempt in range(_BLE_CONNECT_ATTEMPTS): + if attempt: + await asyncio.sleep(_BLE_CONNECT_RETRY_DELAY_S) + try: + return await mesh_core.create_ble( + address=self._address, + device=self._ble_device, + pin=self._pin, + default_timeout=self._connect_timeout, + auto_reconnect=False, + ) + except ConnectionError as exc: + _log.debug( + "BLE link to %s failed to open (attempt %d/%d): %s", + self._address, + attempt + 1, + _BLE_CONNECT_ATTEMPTS, + exc, + ) + last_exc = exc + assert last_exc is not None # the loop always runs; only ConnectionError falls through + raise last_exc + async def _pair_ble_windows(self, *, force: bool) -> bool: """Establish an authenticated BLE bond via the WinRT ProvidePin ceremony (Windows only). @@ -1115,13 +1192,46 @@ class MeshCoreDevice(Device): return True # no port recorded (shouldn't happen once connected) — can't tell return serial_port_present(self._port) - async def disconnect(self) -> None: # noqa: D102 - inherited docstring + async def disconnect(self) -> None: + """Close the connection and release resources. Idempotent, and bounded in time. + + The graceful ``meshcore`` teardown is given :data:`_DISCONNECT_TIMEOUT_S` to finish. + That bound matters: the library's dispatcher stop awaits ``queue.join()``, but its + processor task exits after handling at most one event once stopped — so with two or + more events queued at that instant (a routine advert/RX-log burst on a live mesh) + the join deadlocks and a quit would hang until the exit watchdog force-kills the + process. When the graceful path doesn't return in time it is cancelled and the + teardown is forced instead: the dispatcher task is cancelled synchronously and the + raw transport is closed directly (also bounded), so the port/link is still released. + """ if self._mc is None: return - disconnect = getattr(self._mc, "disconnect", None) - if disconnect is not None: - await disconnect() - self._mc = None + mc, self._mc = self._mc, None + disconnect = getattr(mc, "disconnect", None) + if disconnect is None: + return + try: + await asyncio.wait_for(disconnect(), timeout=_DISCONNECT_TIMEOUT_S) + return + except asyncio.TimeoutError: + _log.debug("graceful disconnect timed out; forcing transport teardown") + except Exception as exc: # noqa: BLE001 - teardown must not raise; fall to forced path + _log.debug("graceful disconnect failed (%s); forcing transport teardown", exc) + # Forced teardown: cancel the (possibly wedged) dispatcher task without awaiting the + # deadlocked join, then close the underlying transport so the serial port / BLE link + # is actually released. Every step is best-effort — nothing here may block the exit. + try: + stop = getattr(mc, "stop", None) + if stop is not None: + stop() + except Exception as exc: # noqa: BLE001 - best-effort force-stop + _log.debug("dispatcher force-stop failed: %s", exc) + try: + raw = getattr(getattr(mc, "connection_manager", None), "connection", None) + if raw is not None: + await asyncio.wait_for(raw.disconnect(), timeout=_FORCE_DISCONNECT_TIMEOUT_S) + except Exception as exc: # noqa: BLE001 - the link may already be gone + _log.debug("forced transport close failed: %s", exc) def _require(self): # type: ignore[no-untyped-def] """Return the live client or raise if not connected.""" @@ -2650,6 +2760,7 @@ def make_device( transport: str = "serial", address: Optional[str] = None, pin: Optional[str] = None, + ble_device: Optional[object] = None, ) -> Device: """Construct the appropriate :class:`Device` for the current invocation. @@ -2662,6 +2773,9 @@ def make_device( transport: ``"serial"`` (default) or ``"ble"``. address: Bluetooth address for the BLE transport. Required when ``transport="ble"``. pin: Optional BLE pairing PIN (BLE only). + ble_device: The scanned ``bleak.BLEDevice`` for ``address``, when this session's + discovery produced one (BLE only) — lets the connect skip re-discovering the + peripheral by address (see :class:`MeshCoreDevice`). Returns: A connected-on-enter :class:`Device` instance. @@ -2677,7 +2791,7 @@ def make_device( "No Bluetooth address configured. Pass --ble, pick a device at startup, " "or use --mock." ) - return MeshCoreDevice(transport="ble", address=address, pin=pin) + return MeshCoreDevice(transport="ble", address=address, pin=pin, ble_device=ble_device) if not port: raise ValueError( "No serial port configured. Pass --port, set a profile, or use --mock." @@ -2729,7 +2843,13 @@ async def probe_device( if device.is_ble: timeout = _PROBE_TIMEOUT_BLE_S probe = MeshCoreDevice( - transport="ble", address=device.address, pin=pin, connect_timeout=timeout + transport="ble", + address=device.address, + pin=pin, + connect_timeout=timeout, + # The scan that discovered the device already holds its BLEDevice; connecting + # through it skips the by-address re-discovery that makes BLE startups flaky. + ble_device=device.ble_device, ) else: timeout = _PROBE_TIMEOUT_SERIAL_S diff --git a/meshterm/core/discovery.py b/meshterm/core/discovery.py index 796929b..aee9b56 100644 --- a/meshterm/core/discovery.py +++ b/meshterm/core/discovery.py @@ -80,6 +80,13 @@ class DiscoveredDevice: transport: ``"serial"`` or ``"ble"`` — which connection layer opens this device. address: Bluetooth address for a BLE device (e.g. ``AA:BB:CC:DD:EE:FF``). name: Advertised BLE local name (e.g. ``"MeshCore-Basestation"``); BLE only. + ble_device: The live ``bleak.BLEDevice`` the scan produced; BLE only, and only for + devices discovered *this session* (a remembered device reloads as a bare + address). Handed to the connection layer so it can open the peripheral + directly — connecting by address alone makes bleak re-discover the device with + a fresh internal scan, which on Windows intermittently misses a + slow-advertising companion. Typed ``object`` so ``bleak`` stays optional; never + persisted. """ port: str = "" @@ -93,6 +100,7 @@ class DiscoveredDevice: transport: str = TRANSPORT_SERIAL address: Optional[str] = None name: Optional[str] = None + ble_device: Optional[object] = None @property def is_ble(self) -> bool: @@ -254,6 +262,7 @@ async def discover_ble_devices(timeout: float = BLE_SCAN_TIMEOUT_S) -> list[Disc name=name, description=name, product=name, + ble_device=dev, ) ) devices.sort(key=lambda d: (d.name or "").casefold()) diff --git a/tests/test_disconnect.py b/tests/test_disconnect.py index 5d74f07..e6e856e 100644 --- a/tests/test_disconnect.py +++ b/tests/test_disconnect.py @@ -214,6 +214,71 @@ async def test_create_ble_gives_up_after_one_repair() -> None: assert calls == [False, True] +async def test_create_ble_retries_a_transient_link_failure(monkeypatch) -> None: + """A transport-level ConnectionError is retried once, and the scanned BLEDevice rides along. + + Models the common Windows flake: the first link open misses the (slow-advertising) + peripheral and the meshcore client raises a bare ``ConnectionError``; users learned to + work around it by re-selecting the device — a manual retry — so the connect retries + itself before surfacing the failure. + """ + scanned_device = object() # the BLEDevice the discovery scan produced + attempts: list[dict] = [] + + class _FakeMeshCore: + @staticmethod + async def create_ble(**kwargs): + attempts.append(kwargs) + if len(attempts) == 1: + raise ConnectionError("Failed to connect to device") + return "connected-client" + + monkeypatch.setattr(connection, "_BLE_CONNECT_RETRY_DELAY_S", 0.0) + dev = connection.MeshCoreDevice( + transport="ble", address="AA:BB:CC:DD:EE:FF", ble_device=scanned_device + ) + await _disable_windows_pairing(dev) + assert await dev._create_ble(_FakeMeshCore) == "connected-client" + assert len(attempts) == 2 # failed once, retried once + # Every attempt connects through the already-discovered BLEDevice, never a bare address. + assert all(kw["device"] is scanned_device for kw in attempts) + + +async def test_create_ble_gives_up_after_the_retry(monkeypatch) -> None: + """A link that never opens surfaces its ConnectionError after the bounded retries.""" + attempts: list[int] = [] + + class _FakeMeshCore: + @staticmethod + async def create_ble(**kwargs): + attempts.append(1) + raise ConnectionError("Failed to connect to device") + + monkeypatch.setattr(connection, "_BLE_CONNECT_RETRY_DELAY_S", 0.0) + dev = connection.MeshCoreDevice(transport="ble", address="AA:BB:CC:DD:EE:FF") + await _disable_windows_pairing(dev) + with pytest.raises(ConnectionError): + await dev._create_ble(_FakeMeshCore) + assert len(attempts) == connection._BLE_CONNECT_ATTEMPTS + + +async def test_create_ble_never_retries_an_auth_rejection() -> None: + """A PIN/bond rejection is translated on the first attempt — never looped by the retry.""" + attempts: list[int] = [] + + class _FakeMeshCore: + @staticmethod + async def create_ble(**kwargs): + attempts.append(1) + raise BleakGATTProtocolError("Insufficient Authentication") + + dev = connection.MeshCoreDevice(transport="ble", address="00:11:22:33:44:55") + await _disable_windows_pairing(dev) + with pytest.raises(connection.DeviceAuthenticationError): + await dev._create_ble(_FakeMeshCore) + assert len(attempts) == 1 + + async def test_pair_ble_windows_noops_without_pin() -> None: """Pairing is skipped (no WinRT touched) when no PIN is set — the fast, hermetic path.""" dev = connection.MeshCoreDevice(transport="ble", address="00:11:22:33:44:55") @@ -710,3 +775,70 @@ async def test_session_loop_resumes_a_fresh_menu_after_reconnect(monkeypatch) -> await asyncio.wait_for(menu._session_loop(object(), session), timeout=2) assert state["handle"] == 1 # one disconnect handled assert state["menu"] == 2 # a fresh menu ran after reconnect + + +class _WedgedMeshCore: + """A fake meshcore client whose graceful ``disconnect()`` never returns. + + Reproduces the library's dispatcher-stop deadlock (``queue.join()`` with events still + queued after the processor task exited), which used to hang MeshTerm's exit until the + watchdog force-killed the process. Records whether the forced path ran. + """ + + def __init__(self) -> None: + self.force_stopped = False + self.connection_manager = SimpleNamespace(connection=self) + self.raw_closed = False + + async def disconnect(self) -> None: + # Called both as the graceful teardown (via the manager-less attribute lookup on + # the client) and as the raw transport close. The graceful call wedges; the raw + # close is distinguished by the force-stop having run first. + if not self.force_stopped: + await asyncio.Event().wait() # the dispatcher deadlock: never returns + self.raw_closed = True + + def stop(self) -> None: + self.force_stopped = True + + +async def test_disconnect_bounds_a_wedged_client_teardown(monkeypatch) -> None: + """A deadlocked graceful teardown is abandoned and the transport force-closed instead.""" + monkeypatch.setattr(connection, "_DISCONNECT_TIMEOUT_S", 0.05) + monkeypatch.setattr(connection, "_FORCE_DISCONNECT_TIMEOUT_S", 0.5) + + dev = connection.MeshCoreDevice(port="COM_TEST") + wedged = _WedgedMeshCore() + dev._mc = wedged + + await asyncio.wait_for(dev.disconnect(), timeout=2.0) # must not hang + + assert wedged.force_stopped # the dispatcher task was cancelled synchronously + assert wedged.raw_closed # the port/link was still released + assert dev._mc is None # idempotent: a second disconnect is a no-op + + +async def test_disconnect_graceful_path_needs_no_force(monkeypatch) -> None: + """A healthy teardown completes gracefully; the forced path is never entered.""" + + class _HealthyMeshCore: + def __init__(self) -> None: + self.disconnected = False + self.force_stopped = False + self.connection_manager = SimpleNamespace(connection=self) + + async def disconnect(self) -> None: + self.disconnected = True + + def stop(self) -> None: + self.force_stopped = True + + dev = connection.MeshCoreDevice(port="COM_TEST") + healthy = _HealthyMeshCore() + dev._mc = healthy + + await dev.disconnect() + + assert healthy.disconnected + assert not healthy.force_stopped + assert dev._mc is None diff --git a/tests/test_discovery.py b/tests/test_discovery.py index 90db7c2..5063365 100644 --- a/tests/test_discovery.py +++ b/tests/test_discovery.py @@ -183,6 +183,10 @@ async def test_discover_ble_filters_to_meshcore(monkeypatch: pytest.MonkeyPatch) devices = await discovery.discover_ble_devices(timeout=0.0) assert [d.name for d in devices] == ["MeshCore-Base"] assert devices[0].is_ble and devices[0].address == "AA:BB:CC:DD:EE:FF" + # The scan's live BLEDevice rides along so the connect can open the peripheral directly + # instead of re-discovering the address (the flaky path on Windows). + assert devices[0].ble_device is not None + assert devices[0].ble_device.address == "AA:BB:CC:DD:EE:FF" async def test_discover_ble_survives_no_adapter(monkeypatch: pytest.MonkeyPatch) -> None: