From 00135bbb9563353054fb946c276b67ff576a27d5 Mon Sep 17 00:00:00 2001
From: agessaman <adam@gessaman.com>
Date: Sun, 26 Jul 2026 19:24:03 -0700
Subject: [PATCH] fix(ble): serialise writes to the RX characteristic

Two overlapping write_gatt_char() calls on the same characteristic drop the
BLE link outright. Observed on macOS/CoreBluetooth as "BLE write failed: 19",
after which the connection is gone and the pending command never completes.

Nothing above the transport guaranteed callers were sequential: schedulers,
health checks, periodic status queries and user commands all issue commands
independently, so any unlucky overlap could take the radio down. The existing
_mesh_request_lock only guards a few binary-request helpers, not the transport.

Reproduced on a companion radio over BLE by issuing send_device_query() and
send_node_discover_req() concurrently:

  before:  BLE write failed: 19, connected=False, command hung >45s
  after:   both complete in 0.12s, connected=True

Issued sequentially the same two commands take 0.09s each and are fine, so it
is specifically the overlap. Ruled out as causes beforehand: notification load
(three discovers under a 91-packet firehose kept writes at 0.06-0.17s with the
link stable) and the discover command itself.

The lock is created lazily so it binds to the running loop, and is released on
the failure path so one failed write cannot wedge every later command.
---
 src/meshcore/ble_cx.py            | 13 ++++++-
 tests/unit/test_error_handling.py | 64 +++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/src/meshcore/ble_cx.py b/src/meshcore/ble_cx.py
index 0a7380f..7cee63f 100644
--- a/src/meshcore/ble_cx.py
+++ b/src/meshcore/ble_cx.py
@@ -4,6 +4,7 @@ mccli.py : CLI interface to MeschCore BLE companion app
 
 import asyncio
 import logging
+from typing import Optional
 
 
 # Make bleak optional - only fail if BLE operations are attempted
@@ -52,6 +53,13 @@ class BLEConnection:
         self.rx_char = None
         self._disconnect_callback = None
         self._background_tasks: set[asyncio.Task] = set()
+        # Serialises write_gatt_char(). Two overlapping writes to the same
+        # characteristic drop the link outright (observed on macOS/CoreBluetooth:
+        # "BLE write failed: 19", connection gone). Nothing above this layer
+        # guarantees callers are sequential -- schedulers, health checks and
+        # user commands all issue commands independently -- so the transport has
+        # to enforce it. Lazily created so it binds to the running loop.
+        self._write_lock: Optional[asyncio.Lock] = None
 
     def _spawn_background(self, coro) -> asyncio.Task:
         """Create a tracked background task (prevents GC of fire-and-forget tasks)."""
@@ -199,8 +207,11 @@ class BLEConnection:
         if not self.rx_char:
             logger.error("RX characteristic not found")
             return False
+        if self._write_lock is None:
+            self._write_lock = asyncio.Lock()
         try:
-            await self.client.write_gatt_char(self.rx_char, bytes(data), response=True)
+            async with self._write_lock:
+                await self.client.write_gatt_char(self.rx_char, bytes(data), response=True)
         except Exception as exc:
             logger.warning(f"BLE write failed: {exc}")
             if self._disconnect_callback:
diff --git a/tests/unit/test_error_handling.py b/tests/unit/test_error_handling.py
index 8be06d1..2470dfe 100644
--- a/tests/unit/test_error_handling.py
+++ b/tests/unit/test_error_handling.py
@@ -412,3 +412,67 @@ async def test_send_trace_unknown_path_hash_len(
 
     assert result.is_error()
     assert result.payload["reason"] == "invalid_path_format"
+
+
+# ── BLE transport serialises writes ──────────────────────────
+
+async def test_ble_send_serialises_concurrent_writes():
+    """Overlapping write_gatt_char() calls must not interleave.
+
+    Two concurrent writes to the same characteristic drop the link outright
+    (observed on macOS/CoreBluetooth: "BLE write failed: 19"). Nothing above the
+    transport guarantees callers are sequential, so the transport must.
+    """
+    from meshcore.ble_cx import BLEConnection
+
+    conn = BLEConnection.__new__(BLEConnection)   # bypass bleak availability check
+    conn._disconnect_callback = None
+    conn._write_lock = None
+    conn.rx_char = object()
+
+    overlap = {"current": 0, "max": 0}
+
+    class FakeClient:
+        async def write_gatt_char(self, char, data, response=True):
+            overlap["current"] += 1
+            overlap["max"] = max(overlap["max"], overlap["current"])
+            await asyncio.sleep(0.01)     # a real write is not instantaneous
+            overlap["current"] -= 1
+
+    conn.client = FakeClient()
+
+    await asyncio.gather(*(conn.send(b"\x01\x02") for _ in range(8)))
+
+    assert overlap["max"] == 1, f"{overlap['max']} writes overlapped"
+
+
+async def test_ble_send_releases_lock_on_write_failure():
+    """A failed write must not wedge the lock for every later command."""
+    from meshcore.ble_cx import BLEConnection
+
+    conn = BLEConnection.__new__(BLEConnection)
+    conn._write_lock = None
+    conn.rx_char = object()
+    reasons = []
+
+    async def capture(reason):
+        reasons.append(reason)
+
+    conn._disconnect_callback = capture
+
+    class BoomThenOK:
+        def __init__(self):
+            self.calls = 0
+
+        async def write_gatt_char(self, char, data, response=True):
+            self.calls += 1
+            if self.calls == 1:
+                raise RuntimeError("19")
+
+    conn.client = BoomThenOK()
+
+    await conn.send(b"\x01")
+    assert reasons == ["ble_write_failed: 19"]
+    # Second write must still be able to acquire the lock.
+    await asyncio.wait_for(conn.send(b"\x02"), timeout=1.0)
+    assert conn.client.calls == 2