From 337792d4d14cb48aa68e580872a264e7350cd7ad Mon Sep 17 00:00:00 2001 From: Jean-Pierre Martineau Date: Mon, 24 Aug 2026 02:03:17 -0400 Subject: [PATCH] BLE: closing the client is not closing the link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Owning the meshcore client was necessary but not sufficient, and the device said so: the same probe still went off the air. ConnectionManager.disconnect closes its transport only if _is_connected, a flag it sets *after* connection.connect() returns. The GATT auth error is thrown from start_notify, long after bleak brought the link up and well before the manager records anything — so the manager is certain there is nothing to close while BLEConnection is still holding a live, connected BleakClient. Calling mc.disconnect() and stopping there looks exactly like a teardown and leaks the link anyway. So _discard_meshcore now runs both halves: the graceful call, which stops the dispatcher and cancels any reconnect task, then the transport closed directly, which is what actually drops the link. BLEConnection.disconnect re-checks client.is_connected, so the second close is a no-op whenever the first one sufficed. That forced path already existed inside disconnect() for the dispatcher's queue.join() deadlock; it is factored out as _force_close_transport and shared, so the two teardowns cannot drift. The tests grow a fake that models the manager's guard rather than counting disconnect() calls, so they assert the link went down instead of asserting a method was invoked — the distinction the first attempt failed on. A test named for that mistake pins it; reverting to graceful-only fails seven of the ten. Verified on hardware, instrumented: the auth path now shows is_connected True -> False inside the discard, the client ends at None, and the peripheral keeps advertising. Full lifecycle clean in one session with no power-cycle — PIN-less probe, PIN retry, bonded session with real traffic, mid-session bond query, Unpair & quit, and a relaunch after it. Co-Authored-By: Claude Opus 5 (1M context) --- meshterm/core/connection.py | 76 +++++++++++----- tests/test_ble_pairing.py | 172 ++++++++++++++++++++++++++---------- 2 files changed, 179 insertions(+), 69 deletions(-) diff --git a/meshterm/core/connection.py b/meshterm/core/connection.py index 4953c5d..4db0ec2 100644 --- a/meshterm/core/connection.py +++ b/meshterm/core/connection.py @@ -116,10 +116,11 @@ _DISCONNECT_TIMEOUT_S = 2.0 #: ``_DISCONNECT_TIMEOUT_S + _FORCE_DISCONNECT_TIMEOUT_S`` stays under the exit watchdog. _FORCE_DISCONNECT_TIMEOUT_S = 1.5 -#: Bound on closing a meshcore client whose own ``connect`` failed (seconds). Deliberately -#: short: this runs on a failure path the user is waiting through — the startup probe that is -#: about to raise "needs a PIN" — and a half-open client has no session state worth draining. -_DISCARD_TIMEOUT_S = 3.0 +#: Bound on the graceful half of closing a meshcore client whose own ``connect`` failed +#: (seconds). Deliberately short: this runs on a failure path the user is waiting through — +#: the startup probe that is about to raise "needs a PIN" — and a half-open client has no +#: session state worth draining. The forced transport close follows regardless. +_DISCARD_TIMEOUT_S = 2.0 #: 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 @@ -1106,6 +1107,21 @@ class MeshCoreDevice(Device): async def _discard_meshcore(mc) -> None: # type: ignore[no-untyped-def] """Close a client whose ``connect`` didn't complete. Bounded, and never raises. + The graceful ``mc.disconnect()`` is **not sufficient on its own here**, and that is + the whole subtlety of this path. ``ConnectionManager.disconnect`` closes the + transport only ``if self._is_connected`` — a flag it sets *after* + ``connection.connect()`` returns. A connect that raised (the GATT authentication + error, thrown from ``start_notify`` well after bleak brought the link up) never got + that far, so the manager is certain there is nothing to close while ``BLEConnection`` + is still holding a live, connected ``BleakClient``. Calling only the graceful path + therefore looks like a teardown and leaks the link anyway — which is exactly how the + first attempt at this fix still left the peripheral off the air. + + So both halves run: the graceful call first (it stops the dispatcher and cancels any + reconnect task), then the transport closed directly, which is what actually drops the + link. ``BLEConnection.disconnect`` re-checks ``client.is_connected``, so the second + close is a no-op whenever the first one did the job. + Shielded on purpose. The other way into this method is a probe whose ``wait_for`` expired and cancelled the handshake mid-flight; a plain ``await`` would then be cancelled itself the moment it suspended, abandoning the very teardown it was called @@ -1115,15 +1131,46 @@ class MeshCoreDevice(Device): Args: mc: A half-open ``meshcore.MeshCore`` client. """ - closing = asyncio.ensure_future( - asyncio.wait_for(mc.disconnect(), timeout=_DISCARD_TIMEOUT_S) - ) + + async def _close() -> None: + try: + await asyncio.wait_for(mc.disconnect(), timeout=_DISCARD_TIMEOUT_S) + except Exception as exc: # noqa: BLE001 - the forced close is the real one + _log.debug("graceful discard failed (%s); forcing the transport close", exc) + await MeshCoreDevice._force_close_transport(mc) + + closing = asyncio.ensure_future(_close()) try: await asyncio.shield(closing) except BaseException as exc: # noqa: BLE001 - teardown of a doomed client # Includes CancelledError: swallowed here only so the caller's own exception # (or cancellation) is the one that propagates. ``closing`` runs on regardless. - _log.debug("discarded a half-open BLE client: %s", exc) + _log.debug("discarding a half-open BLE client: %s", exc) + + @staticmethod + async def _force_close_transport(mc) -> None: # type: ignore[no-untyped-def] + """Cancel the dispatcher and close the raw transport directly. Best-effort, silent. + + The escape hatch from both library traps: a dispatcher stop that deadlocks on its own + ``queue.join()``, and a connection manager that refuses to close a transport it never + recorded as connected. Reaching past both is what actually releases the serial port or + the BLE link. + + Args: + mc: The ``meshcore.MeshCore`` client to tear down. + """ + 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) async def _pair_ble_windows(self, *, force: bool) -> bool: """Establish an authenticated BLE bond via the WinRT ProvidePin ceremony (Windows only). @@ -1440,18 +1487,7 @@ class MeshCoreDevice(Device): # 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) + await MeshCoreDevice._force_close_transport(mc) def _require(self): # type: ignore[no-untyped-def] """Return the live client or raise if not connected.""" diff --git a/tests/test_ble_pairing.py b/tests/test_ble_pairing.py index f23c573..9f09c6f 100644 --- a/tests/test_ble_pairing.py +++ b/tests/test_ble_pairing.py @@ -8,8 +8,15 @@ and the bleak link it had already opened, orphaned inside the library. MeshTerm' life of the process, and the peripheral, still believing it had a peer, stopped advertising. The PIN dialog that opened next then asked for a code it could no longer deliver. -Everything here runs against fakes standing in for ``meshcore.MeshCore`` and -``meshcore.BLEConnection``; no radio, no address, and no pairing code is involved. +Owning the client is only half of it, and the fakes here model the other half. The library's +``ConnectionManager.disconnect`` closes its transport only ``if self._is_connected`` — a flag +set *after* ``connection.connect()`` returns. A connect that raised never set it, so a +graceful ``mc.disconnect()`` walks away from a live ``BleakClient`` believing there is +nothing to close. That is why ``_discard_meshcore`` also closes the transport directly, and +why the assertions below are about the *transport*, not about ``mc.disconnect`` being called. + +Everything runs against stand-ins for ``meshcore.MeshCore`` and ``meshcore.BLEConnection``; +no radio, no real address, and no real pairing code is involved. """ from __future__ import annotations @@ -22,61 +29,87 @@ import pytest from meshterm.core import connection as conn_mod from meshterm.core.connection import MeshCoreDevice -# A stand-in address. Deliberately in the documentation-style ``00:`` space so no real -# companion is named here, and nothing in this file needs hardware to run. +# A stand-in address. Deliberately not a real companion's: a test that names one turns a +# personal device into repository content, and nothing here needs hardware to run. _ADDR = "00:11:22:33:44:55" -class _FakeBLEConnection: - """Stands in for ``meshcore.BLEConnection`` — records how it was constructed.""" +class _FakeTransport: + """Stands in for ``meshcore.BLEConnection`` — what actually holds the bleak client. - last: Optional["_FakeBLEConnection"] = None + ``link_open`` is the thing that matters: it stands for the OS-level link that, left up, + makes the peripheral believe it still has a peer and stop advertising. + """ + + last: Optional["_FakeTransport"] = None def __init__(self, address=None, device=None, pin=None) -> None: self.address = address self.device = device self.pin = pin - _FakeBLEConnection.last = self + self.link_open = False + self.disconnect_calls = 0 + _FakeTransport.last = self + + async def disconnect(self) -> None: + self.disconnect_calls += 1 + self.link_open = False + + +class _FakeConnectionManager: + """Mirrors the library's guard: it will not close a transport it never saw connect.""" + + def __init__(self, connection: _FakeTransport) -> None: + self.connection = connection + self._is_connected = False + + async def disconnect(self) -> None: + if self._is_connected: # the trap: False whenever connection.connect() raised + await self.connection.disconnect() + self._is_connected = False class _FakeMeshCore: """Stands in for the ``meshcore.MeshCore`` *class*, one instance per connect attempt. - ``outcome`` drives what ``connect()`` does: ``"ok"`` returns a truthy handshake result, - ``"none"`` returns ``None`` (transport up, no identity reply), and an exception instance - is raised — the GATT authentication failure being the case that mattered. + ``outcomes`` drives each successive ``connect()``: an exception instance is raised (with + the link already up, as the real GATT failure leaves it), ``"none"`` returns ``None`` + (transport up, no identity reply), ``"slow"`` hangs for a caller to cancel, and anything + else is returned as a successful handshake. """ #: Every instance built during a test, in order (a retry builds a second one). built: list["_FakeMeshCore"] = [] - #: What the next ``connect()`` should do; a list is consumed one entry per attempt. + #: Consumed one entry per connect attempt. outcomes: list = [] def __init__(self, cx, *, default_timeout=None, auto_reconnect=False, **kwargs) -> None: self.cx = cx + self.connection_manager = _FakeConnectionManager(cx) self.default_timeout = default_timeout self.auto_reconnect = auto_reconnect - self.connected = False - self.disconnect_calls = 0 - self.commands = None + self.graceful_calls = 0 + self.stop_calls = 0 type(self).built.append(self) async def connect(self): outcome = type(self).outcomes.pop(0) + self.cx.link_open = True # bleak is connected by the time anything below fails if isinstance(outcome, BaseException): - # The link is up by the time the subscribe fails — that is the whole point: - # something must close it, and only the holder of this object can. - self.connected = True + # The GATT subscribe fails here — after the link is up and *before* the manager + # records it. Nothing downstream will close it unless someone reaches the cx. raise outcome if outcome == "slow": - self.connected = True await asyncio.sleep(30) # a handshake the caller's wait_for will cancel - self.connected = True + self.connection_manager._is_connected = True return None if outcome == "none" else {"ok": True} - async def disconnect(self): - self.disconnect_calls += 1 - self.connected = False + async def disconnect(self) -> None: + self.graceful_calls += 1 + await self.connection_manager.disconnect() + + def stop(self) -> None: + self.stop_calls += 1 @classmethod def reset(cls, *outcomes) -> None: @@ -91,10 +124,10 @@ def _fake_meshcore(monkeypatch: pytest.MonkeyPatch): import types module = types.ModuleType("meshcore") - module.BLEConnection = _FakeBLEConnection + module.BLEConnection = _FakeTransport module.MeshCore = _FakeMeshCore monkeypatch.setitem(sys.modules, "meshcore", module) - _FakeBLEConnection.last = None + _FakeTransport.last = None yield @@ -106,11 +139,11 @@ class _AuthError(Exception): """A stand-in for the GATT failure a PIN-protected companion answers with.""" -def test_a_raising_connect_closes_the_client_it_leaves_behind() -> None: +def test_a_raising_connect_releases_the_link_it_left_open() -> None: """THE bug: a connect that raises must not strand an open link. - ``create_ble`` would have swallowed the reference here. Owning the client means the - failure path can put the link down before the exception continues on its way — so the + ``create_ble`` would have swallowed the reference here. Owning the client lets the + failure path put the link down before the exception continues on its way — so the peripheral stops holding a phantom peer and goes on advertising for the PIN retry. """ _FakeMeshCore.reset(_AuthError("Insufficient Authentication")) @@ -120,18 +153,39 @@ def test_a_raising_connect_closes_the_client_it_leaves_behind() -> None: asyncio.run(dev._connect_owned_ble(_FakeMeshCore)) assert len(_FakeMeshCore.built) == 1 + transport = _FakeMeshCore.built[0].cx + assert transport.link_open is False, "the link was left up — the peripheral goes deaf" + assert transport.disconnect_calls == 1 + + +def test_the_graceful_disconnect_alone_would_not_have_been_enough() -> None: + """The regression guard for the *first* attempt at this fix, which still leaked. + + ``ConnectionManager.disconnect`` is a no-op after a raising connect, because the flag it + guards on is set only once ``connection.connect()`` has returned. Calling ``mc.disconnect`` + and stopping there looks like a teardown and closes nothing — so this asserts the graceful + call really is inert on this path, and that the link went down anyway. + """ + _FakeMeshCore.reset(_AuthError("Insufficient Authentication")) + dev = _device() + + with pytest.raises(_AuthError): + asyncio.run(dev._connect_owned_ble(_FakeMeshCore)) + client = _FakeMeshCore.built[0] - assert client.disconnect_calls == 1, "the half-open client was left open" - assert client.connected is False + assert client.graceful_calls == 1 # it was tried… + assert client.connection_manager._is_connected is False # …and the guard refused it + assert client.cx.link_open is False # …so the direct transport close is what saved us + assert client.stop_calls == 1 # and the dispatcher was cancelled rather than awaited -def test_an_unanswered_handshake_closes_the_client_too() -> None: - """Transport up, no identity reply: reported as "not a companion", link still closed.""" +def test_an_unanswered_handshake_closes_the_link_too() -> None: + """Transport up, no identity reply: reported as "not a companion", link still released.""" _FakeMeshCore.reset("none") dev = _device() assert asyncio.run(dev._connect_owned_ble(_FakeMeshCore)) is None - assert _FakeMeshCore.built[0].disconnect_calls == 1 + assert _FakeMeshCore.built[0].cx.link_open is False def test_a_good_connect_hands_the_client_over_still_open() -> None: @@ -142,8 +196,8 @@ def test_a_good_connect_hands_the_client_over_still_open() -> None: client = asyncio.run(dev._connect_owned_ble(_FakeMeshCore)) assert client is _FakeMeshCore.built[0] - assert client.disconnect_calls == 0 - assert client.connected is True + assert client.graceful_calls == 0 + assert client.cx.link_open is True def test_the_connection_is_built_from_this_device_s_own_endpoint() -> None: @@ -157,7 +211,7 @@ def test_the_connection_is_built_from_this_device_s_own_endpoint() -> None: asyncio.run(dev._connect_owned_ble(_FakeMeshCore)) - cx = _FakeBLEConnection.last + cx = _FakeTransport.last assert (cx.address, cx.device, cx.pin) == (_ADDR, sentinel, "000000") client = _FakeMeshCore.built[0] # auto_reconnect stays off: MeshTerm drives reconnection itself. @@ -177,19 +231,19 @@ def test_a_cancelled_handshake_still_closes_the_link() -> None: with pytest.raises(asyncio.TimeoutError): await asyncio.wait_for(dev._connect_owned_ble(_FakeMeshCore), timeout=0.1) # The shielded close runs on after the cancellation propagates; give it a tick. - await asyncio.sleep(0.1) + await asyncio.sleep(0.2) return _FakeMeshCore.built[0] client = asyncio.run(scenario()) - assert client.disconnect_calls == 1, "a cancelled handshake stranded its link" + assert client.cx.link_open is False, "a cancelled handshake stranded its link" -def test_a_failing_disconnect_never_masks_the_real_error() -> None: +def test_a_failing_teardown_never_masks_the_real_error() -> None: """Teardown is best-effort: the connect's own failure is what the caller must see.""" class _Stubborn(_FakeMeshCore): async def disconnect(self): - self.disconnect_calls += 1 + self.graceful_calls += 1 raise RuntimeError("teardown exploded") _Stubborn.reset(_AuthError("Insufficient Authentication")) @@ -197,23 +251,43 @@ def test_a_failing_disconnect_never_masks_the_real_error() -> None: with pytest.raises(_AuthError): asyncio.run(dev._connect_owned_ble(_Stubborn)) - assert _Stubborn.built[0].disconnect_calls == 1 + # The graceful half blew up, and the forced close still ran and did the job. + assert _Stubborn.built[0].cx.link_open is False -def test_the_retry_loop_never_reuses_a_torn_down_client() -> None: - """A retried link-open builds a fresh client, and the abandoned one was closed.""" - _FakeMeshCore.reset(ConnectionError("Failed to connect to device"), "ok") +def test_a_wedged_teardown_is_not_waited_out_forever() -> None: + """A dispatcher stop that deadlocks is abandoned, and the transport closed regardless.""" + + class _Wedged(_FakeMeshCore): + async def disconnect(self): + self.graceful_calls += 1 + await asyncio.sleep(30) # the library's queue.join() deadlock + + _Wedged.reset(_AuthError("Insufficient Authentication")) dev = _device() async def scenario(): - return await dev._create_ble_with_retry(_FakeMeshCore) + with pytest.raises(_AuthError): + await dev._connect_owned_ble(_Wedged) + return _Wedged.built[0] - client = asyncio.run(scenario()) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(conn_mod, "_DISCARD_TIMEOUT_S", 0.1) + client = asyncio.run(scenario()) + assert client.cx.link_open is False + + +def test_the_retry_loop_never_reuses_a_torn_down_client() -> None: + """A retried link-open builds a fresh client, and the abandoned one was released.""" + _FakeMeshCore.reset(ConnectionError("Failed to connect to device"), "ok") + dev = _device() + + client = asyncio.run(dev._create_ble_with_retry(_FakeMeshCore)) assert len(_FakeMeshCore.built) == 2, "the retry did not build its own client" - assert _FakeMeshCore.built[0].disconnect_calls == 1, "the failed attempt leaked" + assert _FakeMeshCore.built[0].cx.link_open is False, "the failed attempt leaked" assert client is _FakeMeshCore.built[1] - assert client.disconnect_calls == 0 + assert client.cx.link_open is True def test_every_link_open_attempt_failing_raises_the_last_error() -> None: @@ -228,4 +302,4 @@ def test_every_link_open_attempt_failing_raises_the_last_error() -> None: asyncio.run(dev._create_ble_with_retry(_FakeMeshCore)) assert len(_FakeMeshCore.built) == conn_mod._BLE_CONNECT_ATTEMPTS - assert all(c.disconnect_calls == 1 for c in _FakeMeshCore.built) + assert all(not c.cx.link_open for c in _FakeMeshCore.built)