BLE: closing the client is not closing the link

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) <noreply@anthropic.com>

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."""

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)