From d252528fadbe91f2fd505b92647ea48897d711fe Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Sun, 26 Jul 2026 16:17:56 +0000 Subject: [PATCH 01/18] fix: clear the CCCD when stopping notifications On proxies advertising REMOTE_CACHING (connection v3) the esp32 wipes the resolved descriptors to save memory, so the host writes the CCCD itself to enable notifications. Nothing undid that write: the firmware's notify_characteristic() only calls esp_ble_gattc_register_for_notify / esp_ble_gattc_unregister_for_notify, neither of which touches the CCCD. stop_notify() therefore dropped the proxy-side subscription while the peripheral kept notifying for the life of the connection -- draining its battery, spending airtime, and leaving devices that gate behaviour on subscription state stuck in streaming mode. Mirror the start_notify branch and write 0x0000 to the CCCD before releasing the proxy subscription. The release happens in a finally block so a failing CCCD write cannot leave the local bookkeeping drifted from the already-popped handle. --- src/bleak_esphome/backend/client.py | 30 ++++++++- tests/backend/test_client_branches.py | 96 ++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 3 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 6668142..58157e7 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -60,6 +60,7 @@ CCCD_UUID = "00002902-0000-1000-8000-00805f9b34fb" CCCD_NOTIFY_BYTES = b"\x01\x00" CCCD_INDICATE_BYTES = b"\x02\x00" +CCCD_DISABLE_BYTES = b"\x00\x00" DEFAULT_MAX_WRITE_WITHOUT_RESPONSE = DEFAULT_MTU - GATT_HEADER_SIZE @@ -961,8 +962,33 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: self._raise_if_not_connected() # Do not raise KeyError if notifications are not enabled on this characteristic # to be consistent with the behavior of the BlueZ backend - if notify_cancel := self._notify_cancels.pop(characteristic.handle, None): - notify_stop, _ = notify_cancel + if not (notify_cancel := self._notify_cancels.pop(characteristic.handle, None)): + return + notify_stop, _ = notify_cancel + try: + # Mirror of the connection v3 branch in start_notify: the esp32 + # never resolved the descriptors, so it cannot undo the CCCD + # write either. Without this the proxy stops forwarding the + # notifications but the peripheral keeps sending them for the + # life of the connection. Write the CCCD first so the peripheral + # is quiet before the proxy-side subscription goes away. + if self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value and ( + cccd_descriptor := characteristic.get_descriptor(CCCD_UUID) + ): + _LOGGER.debug( + "%s: Writing to CCD descriptor %s to stop notifications", + self._description, + cccd_descriptor.handle, + ) + await self._client.bluetooth_gatt_write_descriptor( + self._address_as_int, + cccd_descriptor.handle, + CCCD_DISABLE_BYTES, + ) + finally: + # Always release the proxy-side subscription, even when the CCCD + # write fails, so the local bookkeeping cannot drift from the + # already-popped handle. await notify_stop() def _raise_if_not_connected(self) -> None: diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index 692ea79..a90bee7 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -35,7 +35,11 @@ from bleak.exc import BleakError from pytest_asyncio import fixture as aio_fixture -from bleak_esphome.backend.client import ESPHomeClient, ESPHomeClientData +from bleak_esphome.backend.client import ( + CCCD_UUID, + ESPHomeClient, + ESPHomeClientData, +) from ._helpers import ESP_MAC_ADDRESS, _make_client @@ -473,12 +477,102 @@ async def test_stop_notify_calls_stop_callback( abort = Mock() char = Mock() char.handle = 99 + char.get_descriptor.return_value = None client._notify_cancels[99] = (stop, abort) await client.stop_notify(char) stop.assert_awaited_once() assert 99 not in client._notify_cancels +@pytest.mark.asyncio +async def test_stop_notify_disables_cccd( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """With REMOTE_CACHING the host clears the CCCD it wrote on start.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + cccd = char.get_descriptor(CCCD_UUID) + assert cccd is not None + stop = AsyncMock() + client._notify_cancels[char.handle] = (stop, Mock()) + with patch.object( + client._client, "bluetooth_gatt_write_descriptor" + ) as mock_write_desc: + await client.stop_notify(char) + mock_write_desc.assert_awaited_once_with( + client._address_as_int, cccd.handle, b"\x00\x00" + ) + stop.assert_awaited_once() + assert char.handle not in client._notify_cancels + + +@pytest.mark.asyncio +async def test_stop_notify_skips_cccd_without_remote_caching( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """Without REMOTE_CACHING the esp32 owns the CCCD, so the host leaves it.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + client._feature_flags &= ~BluetoothProxyFeature.REMOTE_CACHING.value + stop = AsyncMock() + client._notify_cancels[char.handle] = (stop, Mock()) + with patch.object( + client._client, "bluetooth_gatt_write_descriptor" + ) as mock_write_desc: + await client.stop_notify(char) + mock_write_desc.assert_not_called() + stop.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_stop_notify_releases_proxy_when_cccd_write_fails( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A failing CCCD write still releases the proxy-side subscription.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + stop = AsyncMock() + client._notify_cancels[char.handle] = (stop, Mock()) + with ( + patch.object( + client._client, + "bluetooth_gatt_write_descriptor", + side_effect=BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)), + ), + pytest.raises(BleakError), + ): + await client.stop_notify(char) + stop.assert_awaited_once() + assert char.handle not in client._notify_cancels + + @pytest.mark.asyncio async def test_stop_notify_missing_handle_is_noop( client_data: ESPHomeClientData, From 999f4812054a32973a58b3039f95dde2dc3d6ca0 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Tue, 18 Aug 2026 18:00:51 +0000 Subject: [PATCH 02/18] fix: warn on missing CCCD and keep the write error on stop_notify --- src/bleak_esphome/backend/client.py | 51 +++++++++++++++-------- tests/backend/test_client_branches.py | 58 +++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 16 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 58157e7..9749eeb 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -972,24 +972,43 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: # notifications but the peripheral keeps sending them for the # life of the connection. Write the CCCD first so the peripheral # is quiet before the proxy-side subscription goes away. - if self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value and ( - cccd_descriptor := characteristic.get_descriptor(CCCD_UUID) - ): - _LOGGER.debug( - "%s: Writing to CCD descriptor %s to stop notifications", - self._description, - cccd_descriptor.handle, - ) - await self._client.bluetooth_gatt_write_descriptor( - self._address_as_int, - cccd_descriptor.handle, - CCCD_DISABLE_BYTES, - ) - finally: + if self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value: + cccd_descriptor = characteristic.get_descriptor(CCCD_UUID) + if not cccd_descriptor: + # start_notify raises when the descriptor is missing, so + # reaching here means it disappeared after subscribing. + # Log instead of raising: the proxy-side subscription + # still has to be released, but staying silent would hide + # a peripheral that keeps notifying forever. + _LOGGER.warning( + "%s: Characteristic %s does not have a characteristic " + "client config descriptor; the peripheral may keep " + "sending notifications", + self._description, + characteristic.uuid, + ) + else: + _LOGGER.debug( + "%s: Writing to CCD descriptor %s to stop notifications", + self._description, + cccd_descriptor.handle, + ) + await self._client.bluetooth_gatt_write_descriptor( + self._address_as_int, + cccd_descriptor.handle, + CCCD_DISABLE_BYTES, + ) + except BaseException: + # Use BaseException to handle CancelledError as well as Exception. # Always release the proxy-side subscription, even when the CCCD # write fails, so the local bookkeeping cannot drift from the - # already-popped handle. - await notify_stop() + # already-popped handle. The release is best-effort here because + # the CCCD failure is the actionable root cause and must not be + # replaced by a secondary error from the cleanup path. + with contextlib.suppress(Exception): + await notify_stop() + raise + await notify_stop() def _raise_if_not_connected(self) -> None: """Raise a BleakError if not connected.""" diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index a90bee7..f84717a 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -13,6 +13,7 @@ import asyncio import gc +import logging from collections.abc import Iterator from typing import Any from unittest.mock import AsyncMock, Mock, patch @@ -573,6 +574,63 @@ async def test_stop_notify_releases_proxy_when_cccd_write_fails( assert char.handle not in client._notify_cancels +@pytest.mark.asyncio +async def test_stop_notify_warns_when_cccd_missing( + client_data: ESPHomeClientData, + caplog: pytest.LogCaptureFixture, +) -> None: + """A missing CCCD under REMOTE_CACHING is logged, not silently skipped.""" + client = _make_client(client_data) + client._is_connected = True + stop = AsyncMock() + char = Mock() + char.handle = 99 + char.uuid = "00002a05-0000-1000-8000-00805f9b34fb" + char.get_descriptor.return_value = None + client._notify_cancels[99] = (stop, Mock()) + with ( + caplog.at_level(logging.WARNING), + patch.object( + client._client, "bluetooth_gatt_write_descriptor" + ) as mock_write_desc, + ): + await client.stop_notify(char) + mock_write_desc.assert_not_called() + stop.assert_awaited_once() + assert "client config descriptor" in caplog.text + + +@pytest.mark.asyncio +async def test_stop_notify_cccd_failure_survives_failing_release( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A failing release does not mask the CCCD write error.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + stop = AsyncMock(side_effect=RuntimeError("release failed")) + client._notify_cancels[char.handle] = (stop, Mock()) + with ( + patch.object( + client._client, + "bluetooth_gatt_write_descriptor", + side_effect=BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)), + ), + pytest.raises(BleakError), + ): + await client.stop_notify(char) + stop.assert_awaited_once() + assert char.handle not in client._notify_cancels + + @pytest.mark.asyncio async def test_stop_notify_missing_handle_is_noop( client_data: ESPHomeClientData, From 0db9ec539da9b46cb1c31bf8e5a2bdf5ac30b727 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Tue, 18 Aug 2026 19:51:16 +0000 Subject: [PATCH 03/18] fix: log failed proxy notify release and document best-effort CCCD clear --- src/bleak_esphome/backend/client.py | 23 +++++++++++++++-- tests/backend/test_client_branches.py | 36 ++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 9749eeb..0cd4261 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -951,6 +951,13 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: handle, matching the BlueZ backend's behavior. Callers do not need to track which characteristics they have subscribed to. + On connection v3 (``REMOTE_CACHING``) proxies the client config + descriptor is cleared so the peripheral stops notifying. That part + is best-effort: if the descriptor cannot be found it is logged as a + warning and this method still returns successfully, so a successful + return does not guarantee the peripheral stopped sending + notifications. + Args: ---- characteristic (BleakGATTCharacteristic): @@ -1004,9 +1011,21 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: # write fails, so the local bookkeeping cannot drift from the # already-popped handle. The release is best-effort here because # the CCCD failure is the actionable root cause and must not be - # replaced by a secondary error from the cleanup path. - with contextlib.suppress(Exception): + # replaced by a secondary error from the cleanup path -- including + # a CancelledError, which contextlib.suppress(Exception) would let + # escape. Log the failed release so the leaked subscription is not + # invisible: the handle is already popped, so nothing else will + # report it. + try: await notify_stop() + except BaseException: + _LOGGER.warning( + "%s: Failed to release the proxy notify subscription for " + "handle %s; the proxy may keep forwarding notifications", + self._description, + characteristic.handle, + exc_info=True, + ) raise await notify_stop() diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index f84717a..f59c468 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -604,8 +604,9 @@ async def test_stop_notify_warns_when_cccd_missing( async def test_stop_notify_cccd_failure_survives_failing_release( client_data: ESPHomeClientData, esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, + caplog: pytest.LogCaptureFixture, ) -> None: - """A failing release does not mask the CCCD write error.""" + """A failing release does not mask the CCCD write error but is logged.""" client = _make_client(client_data) client._is_connected = True with patch.object( @@ -618,6 +619,39 @@ async def test_stop_notify_cccd_failure_survives_failing_release( assert char is not None stop = AsyncMock(side_effect=RuntimeError("release failed")) client._notify_cancels[char.handle] = (stop, Mock()) + with ( + caplog.at_level(logging.WARNING), + patch.object( + client._client, + "bluetooth_gatt_write_descriptor", + side_effect=BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)), + ), + pytest.raises(BleakError), + ): + await client.stop_notify(char) + stop.assert_awaited_once() + assert char.handle not in client._notify_cancels + assert "Failed to release the proxy notify subscription" in caplog.text + + +@pytest.mark.asyncio +async def test_stop_notify_cccd_failure_survives_cancelled_release( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A cancelled release does not replace the CCCD write error.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + stop = AsyncMock(side_effect=asyncio.CancelledError()) + client._notify_cancels[char.handle] = (stop, Mock()) with ( patch.object( client._client, From f9eed673130444b02e36469d0ef587f66ce08595 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Tue, 18 Aug 2026 21:10:34 +0000 Subject: [PATCH 04/18] fix: propagate cancelled notify release and keep stop_notify retryable --- src/bleak_esphome/backend/client.py | 32 ++++++++++++++++++--------- tests/backend/test_client_branches.py | 29 +++++++++++++++++++----- 2 files changed, 46 insertions(+), 15 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 0cd4261..fe0d123 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -958,6 +958,13 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: return does not guarantee the peripheral stopped sending notifications. + The CCCD write is a round trip to the peripheral, so this method + can block for up to the proxy GATT timeout and can raise + ``BleakError`` where it previously always returned. When it raises, + the subscription entry is kept unless the proxy-side release + succeeded, so calling ``stop_notify`` again retries the part that + failed rather than silently returning. + Args: ---- characteristic (BleakGATTCharacteristic): @@ -969,7 +976,7 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: self._raise_if_not_connected() # Do not raise KeyError if notifications are not enabled on this characteristic # to be consistent with the behavior of the BlueZ backend - if not (notify_cancel := self._notify_cancels.pop(characteristic.handle, None)): + if not (notify_cancel := self._notify_cancels.get(characteristic.handle)): return notify_stop, _ = notify_cancel try: @@ -1008,17 +1015,17 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: except BaseException: # Use BaseException to handle CancelledError as well as Exception. # Always release the proxy-side subscription, even when the CCCD - # write fails, so the local bookkeeping cannot drift from the - # already-popped handle. The release is best-effort here because - # the CCCD failure is the actionable root cause and must not be - # replaced by a secondary error from the cleanup path -- including - # a CancelledError, which contextlib.suppress(Exception) would let - # escape. Log the failed release so the leaked subscription is not - # invisible: the handle is already popped, so nothing else will - # report it. + # write fails. The release is best-effort here because the CCCD + # failure is the actionable root cause and must not be replaced + # by a secondary error from the cleanup path -- except for a + # CancelledError, which is a request to stop and has to win over + # any error it interrupts, so it is deliberately not caught here. try: await notify_stop() - except BaseException: + except Exception: + # Keep the entry so a later stop_notify retries the release + # instead of hitting the missing-handle no-op, and log it + # because the caller only sees the CCCD error. _LOGGER.warning( "%s: Failed to release the proxy notify subscription for " "handle %s; the proxy may keep forwarding notifications", @@ -1026,8 +1033,13 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: characteristic.handle, exc_info=True, ) + else: + del self._notify_cancels[characteristic.handle] raise await notify_stop() + # Popped only once the release succeeded: a failing release leaves the + # entry in place so the caller can retry it. + del self._notify_cancels[characteristic.handle] def _raise_if_not_connected(self) -> None: """Raise a BleakError if not connected.""" diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index f59c468..e4b451b 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -606,7 +606,7 @@ async def test_stop_notify_cccd_failure_survives_failing_release( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, caplog: pytest.LogCaptureFixture, ) -> None: - """A failing release does not mask the CCCD write error but is logged.""" + """A failing release keeps the entry so the caller can retry it.""" client = _make_client(client_data) client._is_connected = True with patch.object( @@ -630,7 +630,7 @@ async def test_stop_notify_cccd_failure_survives_failing_release( ): await client.stop_notify(char) stop.assert_awaited_once() - assert char.handle not in client._notify_cancels + assert char.handle in client._notify_cancels assert "Failed to release the proxy notify subscription" in caplog.text @@ -639,7 +639,7 @@ async def test_stop_notify_cccd_failure_survives_cancelled_release( client_data: ESPHomeClientData, esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: - """A cancelled release does not replace the CCCD write error.""" + """A cancelled release propagates instead of being demoted to a log.""" client = _make_client(client_data) client._is_connected = True with patch.object( @@ -658,11 +658,30 @@ async def test_stop_notify_cccd_failure_survives_cancelled_release( "bluetooth_gatt_write_descriptor", side_effect=BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)), ), - pytest.raises(BleakError), + pytest.raises(asyncio.CancelledError), ): await client.stop_notify(char) stop.assert_awaited_once() - assert char.handle not in client._notify_cancels + assert char.handle in client._notify_cancels + + +@pytest.mark.asyncio +async def test_stop_notify_keeps_entry_when_release_fails( + client_data: ESPHomeClientData, +) -> None: + """A failing release on the success path stays retryable.""" + client = _make_client(client_data) + client._is_connected = True + stop = AsyncMock(side_effect=RuntimeError("release failed")) + char = Mock() + char.handle = 99 + char.uuid = "00002a05-0000-1000-8000-00805f9b34fb" + char.get_descriptor.return_value = None + client._notify_cancels[99] = (stop, Mock()) + with pytest.raises(RuntimeError): + await client.stop_notify(char) + stop.assert_awaited_once() + assert 99 in client._notify_cancels @pytest.mark.asyncio From 306b2f7f9ccad4fa9500f3faf4c2c8694bcc7f59 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Wed, 19 Aug 2026 06:19:32 +0000 Subject: [PATCH 05/18] fix: use pop when clearing notify state and correct the retry docstring --- src/bleak_esphome/backend/client.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index fe0d123..87c2be6 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -960,10 +960,12 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: The CCCD write is a round trip to the peripheral, so this method can block for up to the proxy GATT timeout and can raise - ``BleakError`` where it previously always returned. When it raises, - the subscription entry is kept unless the proxy-side release - succeeded, so calling ``stop_notify`` again retries the part that - failed rather than silently returning. + ``BleakError`` where it previously always returned. Only a failed + proxy-side release is retryable: the subscription entry is kept so a + later ``stop_notify`` retries it. A failed CCCD write is terminal for + that subscription -- the proxy-side release still happens and the + entry is dropped, so a retry returns silently without re-attempting + the descriptor write. Args: ---- @@ -1034,12 +1036,16 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: exc_info=True, ) else: - del self._notify_cancels[characteristic.handle] + # pop() rather than del: a disconnect can clear the dict + # while notify_stop() is awaited, and a bare KeyError here + # would replace the actionable CCCD failure. + self._notify_cancels.pop(characteristic.handle, None) raise await notify_stop() # Popped only once the release succeeded: a failing release leaves the - # entry in place so the caller can retry it. - del self._notify_cancels[characteristic.handle] + # entry in place so the caller can retry it. pop() rather than del + # because a disconnect can clear the dict during the await above. + self._notify_cancels.pop(characteristic.handle, None) def _raise_if_not_connected(self) -> None: """Raise a BleakError if not connected.""" From 59a4e3639f94cead535fea496a2affff10910307 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Wed, 19 Aug 2026 07:07:09 +0000 Subject: [PATCH 06/18] fix: pin the CCCD clear to the standard notify timeout --- docs/usage.md | 2 +- src/bleak_esphome/backend/client.py | 6 ++- tests/backend/test_client_branches.py | 69 ++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index c04e6b9..90d6188 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -327,7 +327,7 @@ requires and what happens when the proxy firmware does not advertise it. | `BleakClient.connect` / GATT operations | `ACTIVE_CONNECTIONS` | The scanner is registered as non-connectable. Discovery still works; connect attempts are rejected by `bleak` before reaching this library. | | Decoded vs. raw advertisements | `RAW_ADVERTISEMENTS` | Falls back to `subscribe_bluetooth_le_advertisements` (proxy-side decoded) instead of `subscribe_bluetooth_le_raw_advertisements`. Both paths feed the same scanner. | | Scanner state / mode + on-demand active windows | `FEATURE_STATE_AND_MODE` | `subscribe_bluetooth_scanner_state` is skipped and the `APIClient` is not bound to the scanner, so `current_mode` / `requested_mode` stay at their defaults and `habluetooth` cannot open on-demand active-scan windows (`async_request_active_window` returns `False`). Connection-slot tracking is unaffected. | -| `connect(dangerous_use_bleak_cache=…)` | `REMOTE_CACHING` | The cached-services hint sent to the proxy is forced off, so the proxy re-discovers services on every connect. `dangerous_use_bleak_cache` still hits the on-host LRU in `_get_services` when populated, and `start_notify` skips the CCCD write because the firmware handles it. | +| `connect(dangerous_use_bleak_cache=…)` | `REMOTE_CACHING` | The cached-services hint sent to the proxy is forced off, so the proxy re-discovers services on every connect. `dangerous_use_bleak_cache` still hits the on-host LRU in `_get_services` when populated, and `start_notify` skips the CCCD write -- and `stop_notify` the matching CCCD clear -- because the firmware handles both. | | `BleakClient.pair` / `unpair` | `PAIRING` | Raises `NotImplementedError("Pairing is not available in this version ESPHome; Upgrade the ESPHome version on the … device.")`. | | `ESPHomeClient.clear_cache` | `CACHE_CLEARING` | Returns `True` after clearing only the on-host LRU caches; logs `"On device cache clear is not available with this ESPHome version; … Only memory cache will be cleared"`. No proxy round-trip. | | `ESPHomeClient.set_connection_params` | `CONNECTION_PARAMS_SETTING` | Silently returns after logging `"Setting connection parameters is not available with ESPHome version …; Upgrade the ESPHome version on the device"`. No exception is raised. | diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 87c2be6..2bc0b01 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -965,7 +965,10 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: later ``stop_notify`` retries it. A failed CCCD write is terminal for that subscription -- the proxy-side release still happens and the entry is dropped, so a retry returns silently without re-attempting - the descriptor write. + the descriptor write. While a failed release is retained it also + blocks ``start_notify`` on that handle, which shares the same + bookkeeping as its duplicate-subscription guard; a disconnect clears + the entry. Args: ---- @@ -1013,6 +1016,7 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: self._address_as_int, cccd_descriptor.handle, CCCD_DISABLE_BYTES, + GATT_NOTIFY_TIMEOUT, ) except BaseException: # Use BaseException to handle CancelledError as well as Exception. diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index e4b451b..0ff6149 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -38,6 +38,7 @@ from bleak_esphome.backend.client import ( CCCD_UUID, + GATT_NOTIFY_TIMEOUT, ESPHomeClient, ESPHomeClientData, ) @@ -510,7 +511,7 @@ async def test_stop_notify_disables_cccd( ) as mock_write_desc: await client.stop_notify(char) mock_write_desc.assert_awaited_once_with( - client._address_as_int, cccd.handle, b"\x00\x00" + client._address_as_int, cccd.handle, b"\x00\x00", GATT_NOTIFY_TIMEOUT ) stop.assert_awaited_once() assert char.handle not in client._notify_cancels @@ -684,6 +685,72 @@ async def test_stop_notify_keeps_entry_when_release_fails( assert 99 in client._notify_cancels +@pytest.mark.asyncio +async def test_stop_notify_survives_concurrent_cancel_clear( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A disconnect clearing the dict during the CCCD write is not a KeyError.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + stop = AsyncMock() + client._notify_cancels[char.handle] = (stop, Mock()) + + async def _clear(*args: Any, **kwargs: Any) -> None: + client._notify_cancels.clear() + + with patch.object( + client._client, "bluetooth_gatt_write_descriptor", side_effect=_clear + ): + await client.stop_notify(char) + stop.assert_awaited_once() + assert char.handle not in client._notify_cancels + + +@pytest.mark.asyncio +async def test_stop_notify_cccd_failure_survives_concurrent_cancel_clear( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """The error path pops defensively when a disconnect cleared the dict.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + + async def _clear_and_raise(*args: Any, **kwargs: Any) -> None: + client._notify_cancels.clear() + raise BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)) + + stop = AsyncMock() + client._notify_cancels[char.handle] = (stop, Mock()) + with ( + patch.object( + client._client, + "bluetooth_gatt_write_descriptor", + side_effect=_clear_and_raise, + ), + pytest.raises(BleakError), + ): + await client.stop_notify(char) + stop.assert_awaited_once() + assert char.handle not in client._notify_cancels + + @pytest.mark.asyncio async def test_stop_notify_missing_handle_is_noop( client_data: ESPHomeClientData, From 94b42679c62b1dd697e8c08df747a07afd6c4413 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 07:07:27 +0000 Subject: [PATCH 07/18] chore(pre-commit.ci): auto fixes --- docs/usage.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index 90d6188..d9cea2e 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -322,15 +322,15 @@ checks these flags before calling proxy-side APIs and degrades gracefully when a flag is missing. The table below maps each public surface to the flag it requires and what happens when the proxy firmware does not advertise it. -| Public surface | Required flag | Behavior when flag is absent | -| ----------------------------------------------- | --------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `BleakClient.connect` / GATT operations | `ACTIVE_CONNECTIONS` | The scanner is registered as non-connectable. Discovery still works; connect attempts are rejected by `bleak` before reaching this library. | -| Decoded vs. raw advertisements | `RAW_ADVERTISEMENTS` | Falls back to `subscribe_bluetooth_le_advertisements` (proxy-side decoded) instead of `subscribe_bluetooth_le_raw_advertisements`. Both paths feed the same scanner. | -| Scanner state / mode + on-demand active windows | `FEATURE_STATE_AND_MODE` | `subscribe_bluetooth_scanner_state` is skipped and the `APIClient` is not bound to the scanner, so `current_mode` / `requested_mode` stay at their defaults and `habluetooth` cannot open on-demand active-scan windows (`async_request_active_window` returns `False`). Connection-slot tracking is unaffected. | -| `connect(dangerous_use_bleak_cache=…)` | `REMOTE_CACHING` | The cached-services hint sent to the proxy is forced off, so the proxy re-discovers services on every connect. `dangerous_use_bleak_cache` still hits the on-host LRU in `_get_services` when populated, and `start_notify` skips the CCCD write -- and `stop_notify` the matching CCCD clear -- because the firmware handles both. | -| `BleakClient.pair` / `unpair` | `PAIRING` | Raises `NotImplementedError("Pairing is not available in this version ESPHome; Upgrade the ESPHome version on the … device.")`. | -| `ESPHomeClient.clear_cache` | `CACHE_CLEARING` | Returns `True` after clearing only the on-host LRU caches; logs `"On device cache clear is not available with this ESPHome version; … Only memory cache will be cleared"`. No proxy round-trip. | -| `ESPHomeClient.set_connection_params` | `CONNECTION_PARAMS_SETTING` | Silently returns after logging `"Setting connection parameters is not available with ESPHome version …; Upgrade the ESPHome version on the device"`. No exception is raised. | +| Public surface | Required flag | Behavior when flag is absent | +| ----------------------------------------------- | --------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `BleakClient.connect` / GATT operations | `ACTIVE_CONNECTIONS` | The scanner is registered as non-connectable. Discovery still works; connect attempts are rejected by `bleak` before reaching this library. | +| Decoded vs. raw advertisements | `RAW_ADVERTISEMENTS` | Falls back to `subscribe_bluetooth_le_advertisements` (proxy-side decoded) instead of `subscribe_bluetooth_le_raw_advertisements`. Both paths feed the same scanner. | +| Scanner state / mode + on-demand active windows | `FEATURE_STATE_AND_MODE` | `subscribe_bluetooth_scanner_state` is skipped and the `APIClient` is not bound to the scanner, so `current_mode` / `requested_mode` stay at their defaults and `habluetooth` cannot open on-demand active-scan windows (`async_request_active_window` returns `False`). Connection-slot tracking is unaffected. | +| `connect(dangerous_use_bleak_cache=…)` | `REMOTE_CACHING` | The cached-services hint sent to the proxy is forced off, so the proxy re-discovers services on every connect. `dangerous_use_bleak_cache` still hits the on-host LRU in `_get_services` when populated, and `start_notify` skips the CCCD write -- and `stop_notify` the matching CCCD clear -- because the firmware handles both. | +| `BleakClient.pair` / `unpair` | `PAIRING` | Raises `NotImplementedError("Pairing is not available in this version ESPHome; Upgrade the ESPHome version on the … device.")`. | +| `ESPHomeClient.clear_cache` | `CACHE_CLEARING` | Returns `True` after clearing only the on-host LRU caches; logs `"On device cache clear is not available with this ESPHome version; … Only memory cache will be cleared"`. No proxy round-trip. | +| `ESPHomeClient.set_connection_params` | `CONNECTION_PARAMS_SETTING` | Silently returns after logging `"Setting connection parameters is not available with ESPHome version …; Upgrade the ESPHome version on the device"`. No exception is raised. | If a method appears to silently do nothing, check the proxy's reported feature flags first — the warning is logged at WARNING level on the From 97770e418146544752637f8ea3ccea0f0168ad9b Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Wed, 19 Aug 2026 14:19:14 +0000 Subject: [PATCH 08/18] fix: raise on a missing CCCD in stop_notify and restore its atomic pop --- src/bleak_esphome/backend/client.py | 99 ++++++++++++++------------- tests/backend/test_client_branches.py | 48 ++++++++++--- 2 files changed, 92 insertions(+), 55 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 2bc0b01..9dc11af 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -952,11 +952,10 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: need to track which characteristics they have subscribed to. On connection v3 (``REMOTE_CACHING``) proxies the client config - descriptor is cleared so the peripheral stops notifying. That part - is best-effort: if the descriptor cannot be found it is logged as a - warning and this method still returns successfully, so a successful - return does not guarantee the peripheral stopped sending - notifications. + descriptor is cleared so the peripheral stops notifying. A missing + descriptor raises ``BleakError`` with the same message + ``start_notify`` uses, so a caller never gets a successful return + while the peripheral keeps notifying. The CCCD write is a round trip to the peripheral, so this method can block for up to the proxy GATT timeout and can raise @@ -967,8 +966,9 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: entry is dropped, so a retry returns silently without re-attempting the descriptor write. While a failed release is retained it also blocks ``start_notify`` on that handle, which shares the same - bookkeeping as its duplicate-subscription guard; a disconnect clears - the entry. + bookkeeping as its duplicate-subscription guard; the release failure + is attached to the raised error as a note so the caller can tell the + two apart. A disconnect clears the entry. Args: ---- @@ -980,8 +980,12 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: """ self._raise_if_not_connected() # Do not raise KeyError if notifications are not enabled on this characteristic - # to be consistent with the behavior of the BlueZ backend - if not (notify_cancel := self._notify_cancels.get(characteristic.handle)): + # to be consistent with the behavior of the BlueZ backend. The entry is popped + # up front so a second concurrent stop_notify on the same handle is the same + # no-op it has always been instead of issuing a duplicate CCCD write and a + # duplicate release; it is put back only when the release fails and is + # therefore worth retrying. + if not (notify_cancel := self._notify_cancels.pop(characteristic.handle, None)): return notify_stop, _ = notify_cancel try: @@ -994,31 +998,25 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: if self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value: cccd_descriptor = characteristic.get_descriptor(CCCD_UUID) if not cccd_descriptor: - # start_notify raises when the descriptor is missing, so - # reaching here means it disappeared after subscribing. - # Log instead of raising: the proxy-side subscription - # still has to be released, but staying silent would hide - # a peripheral that keeps notifying forever. - _LOGGER.warning( - "%s: Characteristic %s does not have a characteristic " - "client config descriptor; the peripheral may keep " - "sending notifications", - self._description, - characteristic.uuid, - ) - else: - _LOGGER.debug( - "%s: Writing to CCD descriptor %s to stop notifications", - self._description, - cccd_descriptor.handle, - ) - await self._client.bluetooth_gatt_write_descriptor( - self._address_as_int, - cccd_descriptor.handle, - CCCD_DISABLE_BYTES, - GATT_NOTIFY_TIMEOUT, + # Same error start_notify raises for this condition: + # returning successfully here would hide a peripheral that + # keeps notifying for the life of the connection. + raise BleakError( + f"{self._description}: Characteristic {characteristic.uuid} " + "does not have a characteristic client config descriptor." ) - except BaseException: + _LOGGER.debug( + "%s: Writing to CCD descriptor %s to stop notifications", + self._description, + cccd_descriptor.handle, + ) + await self._client.bluetooth_gatt_write_descriptor( + self._address_as_int, + cccd_descriptor.handle, + CCCD_DISABLE_BYTES, + GATT_NOTIFY_TIMEOUT, + ) + except BaseException as err: # Use BaseException to handle CancelledError as well as Exception. # Always release the proxy-side subscription, even when the CCCD # write fails. The release is best-effort here because the CCCD @@ -1028,10 +1026,13 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: # any error it interrupts, so it is deliberately not caught here. try: await notify_stop() - except Exception: - # Keep the entry so a later stop_notify retries the release - # instead of hitting the missing-handle no-op, and log it - # because the caller only sees the CCCD error. + except Exception as release_err: + # Put the entry back so a later stop_notify retries the + # release instead of hitting the missing-handle no-op, and + # note the failure on the error the caller sees: while the + # entry is retained it also blocks start_notify on this + # handle, which is not something the CCCD error conveys. + self._notify_cancels[characteristic.handle] = notify_cancel _LOGGER.warning( "%s: Failed to release the proxy notify subscription for " "handle %s; the proxy may keep forwarding notifications", @@ -1039,17 +1040,21 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: characteristic.handle, exc_info=True, ) - else: - # pop() rather than del: a disconnect can clear the dict - # while notify_stop() is awaited, and a bare KeyError here - # would replace the actionable CCCD failure. - self._notify_cancels.pop(characteristic.handle, None) + err.add_note( + "Releasing the proxy notify subscription for handle " + f"{characteristic.handle} also failed with {release_err!r}; " + "the proxy may keep forwarding notifications and " + "start_notify on this handle stays blocked until a later " + "stop_notify succeeds or the device disconnects." + ) + raise + try: + await notify_stop() + except BaseException: + # A failing release is retryable, so put the entry back rather + # than leaving the handle unsubscribed in our bookkeeping only. + self._notify_cancels[characteristic.handle] = notify_cancel raise - await notify_stop() - # Popped only once the release succeeded: a failing release leaves the - # entry in place so the caller can retry it. pop() rather than del - # because a disconnect can clear the dict during the await above. - self._notify_cancels.pop(characteristic.handle, None) def _raise_if_not_connected(self) -> None: """Raise a BleakError if not connected.""" diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index 0ff6149..5027b1d 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -475,11 +475,11 @@ async def test_stop_notify_calls_stop_callback( """``stop_notify`` awaits and removes the stored stop callback.""" client = _make_client(client_data) client._is_connected = True + client._feature_flags &= ~BluetoothProxyFeature.REMOTE_CACHING.value stop = AsyncMock() abort = Mock() char = Mock() char.handle = 99 - char.get_descriptor.return_value = None client._notify_cancels[99] = (stop, abort) await client.stop_notify(char) stop.assert_awaited_once() @@ -576,11 +576,10 @@ async def test_stop_notify_releases_proxy_when_cccd_write_fails( @pytest.mark.asyncio -async def test_stop_notify_warns_when_cccd_missing( +async def test_stop_notify_raises_when_cccd_missing( client_data: ESPHomeClientData, - caplog: pytest.LogCaptureFixture, ) -> None: - """A missing CCCD under REMOTE_CACHING is logged, not silently skipped.""" + """A missing CCCD under REMOTE_CACHING raises, as it does on start.""" client = _make_client(client_data) client._is_connected = True stop = AsyncMock() @@ -590,15 +589,15 @@ async def test_stop_notify_warns_when_cccd_missing( char.get_descriptor.return_value = None client._notify_cancels[99] = (stop, Mock()) with ( - caplog.at_level(logging.WARNING), patch.object( client._client, "bluetooth_gatt_write_descriptor" ) as mock_write_desc, + pytest.raises(BleakError, match="client config descriptor"), ): await client.stop_notify(char) mock_write_desc.assert_not_called() stop.assert_awaited_once() - assert "client config descriptor" in caplog.text + assert 99 not in client._notify_cancels @pytest.mark.asyncio @@ -627,12 +626,14 @@ async def test_stop_notify_cccd_failure_survives_failing_release( "bluetooth_gatt_write_descriptor", side_effect=BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)), ), - pytest.raises(BleakError), + pytest.raises(BleakError) as excinfo, ): await client.stop_notify(char) stop.assert_awaited_once() assert char.handle in client._notify_cancels assert "Failed to release the proxy notify subscription" in caplog.text + notes = getattr(excinfo.value.__cause__, "__notes__", []) + assert any("also failed with" in note for note in notes) @pytest.mark.asyncio @@ -673,11 +674,11 @@ async def test_stop_notify_keeps_entry_when_release_fails( """A failing release on the success path stays retryable.""" client = _make_client(client_data) client._is_connected = True + client._feature_flags &= ~BluetoothProxyFeature.REMOTE_CACHING.value stop = AsyncMock(side_effect=RuntimeError("release failed")) char = Mock() char.handle = 99 char.uuid = "00002a05-0000-1000-8000-00805f9b34fb" - char.get_descriptor.return_value = None client._notify_cancels[99] = (stop, Mock()) with pytest.raises(RuntimeError): await client.stop_notify(char) @@ -715,6 +716,37 @@ async def _clear(*args: Any, **kwargs: Any) -> None: assert char.handle not in client._notify_cancels +@pytest.mark.asyncio +async def test_stop_notify_is_single_winner_when_reentered( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A second stop_notify during the CCCD write is a no-op, not a duplicate.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + stop = AsyncMock() + client._notify_cancels[char.handle] = (stop, Mock()) + + async def _reenter(*args: Any, **kwargs: Any) -> None: + await client.stop_notify(char) + + with patch.object( + client._client, "bluetooth_gatt_write_descriptor", side_effect=_reenter + ) as mock_write_desc: + await client.stop_notify(char) + assert mock_write_desc.await_count == 1 + stop.assert_awaited_once() + assert char.handle not in client._notify_cancels + + @pytest.mark.asyncio async def test_stop_notify_cccd_failure_survives_concurrent_cancel_clear( client_data: ESPHomeClientData, From e9c46585fbd42a5846550e516c2af84635427ec8 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Wed, 19 Aug 2026 15:07:58 +0000 Subject: [PATCH 09/18] fix: resolve CI failures on #410 (attempt 1) --- src/bleak_esphome/backend/client.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 9dc11af..c5a0b71 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -1023,10 +1023,10 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: # failure is the actionable root cause and must not be replaced # by a secondary error from the cleanup path -- except for a # CancelledError, which is a request to stop and has to win over - # any error it interrupts, so it is deliberately not caught here. + # any error it interrupts, so it is re-raised as-is below. try: await notify_stop() - except Exception as release_err: + except BaseException as release_err: # Put the entry back so a later stop_notify retries the # release instead of hitting the missing-handle no-op, and # note the failure on the error the caller sees: while the @@ -1040,6 +1040,11 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: characteristic.handle, exc_info=True, ) + if isinstance(release_err, asyncio.CancelledError): + # A cancellation is a request to stop, so it wins over + # the CCCD error it interrupted instead of being demoted + # to a note on it. + raise err.add_note( "Releasing the proxy notify subscription for handle " f"{characteristic.handle} also failed with {release_err!r}; " From 4301332da66cb14e5e4cb23eb4957434e6a45366 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Wed, 19 Aug 2026 15:22:46 +0000 Subject: [PATCH 10/18] fix: drop the notify entry when a stop_notify release fails after disconnect --- src/bleak_esphome/backend/client.py | 86 ++++++++++++++++++++++----- tests/backend/test_client_branches.py | 65 +++++++++++++++++++- 2 files changed, 135 insertions(+), 16 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index c5a0b71..3f27332 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -8,7 +8,7 @@ import sys from dataclasses import dataclass, field from functools import partial, wraps -from typing import TYPE_CHECKING, Any, Concatenate, ParamSpec, TypeVar +from typing import TYPE_CHECKING, Any, Concatenate, ParamSpec, TypeAlias, TypeVar from aioesphomeapi import ( ESP_CONNECTION_ERROR_DESCRIPTION, @@ -43,6 +43,11 @@ from .device import ESPHomeBluetoothDevice from .scanner import ESPHomeScanner + # (release, abort) pair returned by ``bluetooth_gatt_start_notify``. + _NotifyCancel: TypeAlias = tuple[ + Callable[[], Coroutine[Any, Any, None]], Callable[[], None] + ] + if sys.version_info < (3, 12): from typing_extensions import Buffer else: @@ -69,6 +74,21 @@ _ESPHomeClient = TypeVar("_ESPHomeClient", bound="ESPHomeClient") _R = TypeVar("_R") _P = ParamSpec("_P") +_E = TypeVar("_E", bound=BaseException) + + +def _with_notes(err: _E, cause: BaseException) -> _E: + """ + Carry any notes on ``cause`` over to the error the caller sees. + + The api errors below are re-raised as bleak errors, so a note attached + to the original -- for example ``stop_notify``'s failed-release detail + -- would otherwise only be reachable through ``__cause__`` and be + absent from the traceback of the error the caller actually catches. + """ + for note in getattr(cause, "__notes__", ()): + err.add_note(note) + return err def api_error_as_bleak_error( @@ -84,7 +104,7 @@ async def _async_wrap_bluetooth_operation( try: return await func(self, *args, **kwargs) except TimeoutAPIError as err: - raise TimeoutError(str(err)) from err + raise _with_notes(TimeoutError(str(err)), err) from err except BluetoothConnectionDroppedError as ex: _LOGGER.debug( "%s: BLE device disconnected during %s operation", @@ -92,7 +112,7 @@ async def _async_wrap_bluetooth_operation( func.__name__, ) self._async_ble_device_disconnected() - raise BleakError(str(ex)) from ex + raise _with_notes(BleakError(str(ex)), ex) from ex except BluetoothGATTAPIError as ex: # If the device disconnects in the middle of an operation # be sure to mark it as disconnected so any library using @@ -109,9 +129,9 @@ async def _async_wrap_bluetooth_operation( func.__name__, ) self._async_ble_device_disconnected() - raise BleakError(str(ex)) from ex + raise _with_notes(BleakError(str(ex)), ex) from ex except APIConnectionError as err: - raise BleakError(str(err)) from err + raise _with_notes(BleakError(str(err)), err) from err return _async_wrap_bluetooth_operation @@ -163,9 +183,7 @@ def __init__( self._pending_release = False self._mtu: int | None = None self._cancel_connection_state: Callable[[], None] | None = None - self._notify_cancels: dict[ - int, tuple[Callable[[], Coroutine[Any, Any, None]], Callable[[], None]] - ] = {} + self._notify_cancels: dict[int, _NotifyCancel] = {} self._device_info = client_data.device_info self._feature_flags = device_info.bluetooth_proxy_feature_flags_compat( client_data.api_version @@ -967,8 +985,10 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: the descriptor write. While a failed release is retained it also blocks ``start_notify`` on that handle, which shares the same bookkeeping as its duplicate-subscription guard; the release failure - is attached to the raised error as a note so the caller can tell the - two apart. A disconnect clears the entry. + is attached as a note to the error the caller receives so the two can + be told apart. A release that fails because the device disconnected + drops the entry instead of retaining it, so a reconnect can subscribe + to that handle again. Args: ---- @@ -1032,7 +1052,9 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: # note the failure on the error the caller sees: while the # entry is retained it also blocks start_notify on this # handle, which is not something the CCCD error conveys. - self._notify_cancels[characteristic.handle] = notify_cancel + retained = self._restore_notify_cancel( + characteristic.handle, notify_cancel + ) _LOGGER.warning( "%s: Failed to release the proxy notify subscription for " "handle %s; the proxy may keep forwarding notifications", @@ -1048,9 +1070,14 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: err.add_note( "Releasing the proxy notify subscription for handle " f"{characteristic.handle} also failed with {release_err!r}; " - "the proxy may keep forwarding notifications and " - "start_notify on this handle stays blocked until a later " - "stop_notify succeeds or the device disconnects." + "the proxy may keep forwarding notifications." + + ( + " start_notify on this handle stays blocked until a" + " later stop_notify succeeds or the device" + " disconnects." + if retained + else "" + ) ) raise try: @@ -1058,9 +1085,38 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: except BaseException: # A failing release is retryable, so put the entry back rather # than leaving the handle unsubscribed in our bookkeeping only. - self._notify_cancels[characteristic.handle] = notify_cancel + self._restore_notify_cancel(characteristic.handle, notify_cancel) raise + def _restore_notify_cancel( + self, + handle: int, + notify_cancel: _NotifyCancel, + ) -> bool: + """ + Put a popped notify entry back so a failed release stays retryable. + + Nothing is restored once the device has disconnected: the popped + entry was invisible to ``_async_disconnected_cleanup``, so putting + it back would resurrect state nothing clears again and block + ``start_notify`` on that handle for the life of the client. The + abort half is run instead, mirroring what the cleanup would have + done had the entry still been in the dict. ``setdefault`` keeps a + subscription made after the pop from being clobbered. + + Returns + ------- + ``True`` when the entry is now in the dict, which is also when + it blocks ``start_notify`` on that handle. + + """ + if not self._is_connected: + _, notify_abort = notify_cancel + notify_abort() + return False + self._notify_cancels.setdefault(handle, notify_cancel) + return True + def _raise_if_not_connected(self) -> None: """Raise a BleakError if not connected.""" if not self._is_connected: diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index 5027b1d..c8056d5 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -632,10 +632,73 @@ async def test_stop_notify_cccd_failure_survives_failing_release( stop.assert_awaited_once() assert char.handle in client._notify_cancels assert "Failed to release the proxy notify subscription" in caplog.text - notes = getattr(excinfo.value.__cause__, "__notes__", []) + # The note has to be on the error the caller actually catches, not only + # on the api error the decorator wrapped. + notes = getattr(excinfo.value, "__notes__", []) assert any("also failed with" in note for note in notes) +@pytest.mark.asyncio +async def test_stop_notify_drops_entry_when_release_fails_after_disconnect( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A disconnect during the CCCD write must not resurrect the entry.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + stop = AsyncMock(side_effect=RuntimeError("release failed")) + abort = Mock() + client._notify_cancels[char.handle] = (stop, abort) + + async def _disconnect_and_raise(*args: Any, **kwargs: Any) -> None: + client._async_disconnected_cleanup() + raise BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)) + + with ( + patch.object( + client._client, + "bluetooth_gatt_write_descriptor", + side_effect=_disconnect_and_raise, + ), + pytest.raises(BleakError), + ): + await client.stop_notify(char) + stop.assert_awaited_once() + abort.assert_called_once() + assert char.handle not in client._notify_cancels + + +@pytest.mark.asyncio +async def test_stop_notify_release_failure_keeps_newer_subscription( + client_data: ESPHomeClientData, +) -> None: + """A restored entry never clobbers a subscription made after the pop.""" + client = _make_client(client_data) + client._is_connected = True + client._feature_flags &= ~BluetoothProxyFeature.REMOTE_CACHING.value + newer = (AsyncMock(), Mock()) + + async def _resubscribe() -> None: + client._notify_cancels[99] = newer + raise RuntimeError("release failed") + + char = Mock() + char.handle = 99 + char.uuid = "00002a05-0000-1000-8000-00805f9b34fb" + client._notify_cancels[99] = (_resubscribe, Mock()) + with pytest.raises(RuntimeError): + await client.stop_notify(char) + assert client._notify_cancels[99] is newer + + @pytest.mark.asyncio async def test_stop_notify_cccd_failure_survives_cancelled_release( client_data: ESPHomeClientData, From ef26e9d7daa5c0a17d9e1cca085368df58061e9e Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Wed, 19 Aug 2026 16:28:26 +0000 Subject: [PATCH 11/18] fix: retry a failed CCCD clear and abort notify entries that lose the handle --- src/bleak_esphome/backend/client.py | 160 +++++++++++++++++--------- tests/backend/test_client_branches.py | 68 ++++++++++- 2 files changed, 171 insertions(+), 57 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 3f27332..1c08b57 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -184,6 +184,10 @@ def __init__( self._mtu: int | None = None self._cancel_connection_state: Callable[[], None] | None = None self._notify_cancels: dict[int, _NotifyCancel] = {} + # Handles whose CCCD clear failed after the proxy-side subscription + # was already released, so a later stop_notify retries the write + # instead of returning the missing-handle no-op. + self._cccd_dirty: set[int] = set() self._device_info = client_data.device_info self._feature_flags = device_info.bluetooth_proxy_feature_flags_compat( client_data.api_version @@ -210,6 +214,7 @@ def _async_disconnected_cleanup(self) -> None: for _, notify_abort in self._notify_cancels.values(): notify_abort() self._notify_cancels.clear() + self._cccd_dirty.clear() self._disconnect_callbacks.discard(self._async_esp_disconnected) self._bluetooth_device.async_untrack_client( self._address_as_int, self._async_ble_device_disconnected @@ -977,18 +982,20 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: The CCCD write is a round trip to the peripheral, so this method can block for up to the proxy GATT timeout and can raise - ``BleakError`` where it previously always returned. Only a failed - proxy-side release is retryable: the subscription entry is kept so a - later ``stop_notify`` retries it. A failed CCCD write is terminal for - that subscription -- the proxy-side release still happens and the - entry is dropped, so a retry returns silently without re-attempting - the descriptor write. While a failed release is retained it also - blocks ``start_notify`` on that handle, which shares the same - bookkeeping as its duplicate-subscription guard; the release failure - is attached as a note to the error the caller receives so the two can - be told apart. A release that fails because the device disconnected + ``BleakError`` where it previously always returned. Both failures + are retryable. A failed proxy-side release keeps the subscription + entry so a later ``stop_notify`` retries it; while it is retained it + also blocks ``start_notify`` on that handle, which shares the same + bookkeeping as its duplicate-subscription guard, so the release + failure is attached as a note to the error the caller receives and + the two can be told apart. A failed CCCD write releases the + proxy-side subscription and drops the entry, but the handle is + remembered so a later ``stop_notify`` re-attempts the descriptor + write instead of returning a success while the peripheral keeps + notifying. A release that fails because the device disconnected drops the entry instead of retaining it, so a reconnect can subscribe - to that handle again. + to that handle again; a disconnect likewise forgets any outstanding + CCCD write, since the peripheral stops notifying with the link. Args: ---- @@ -999,51 +1006,35 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: """ self._raise_if_not_connected() + handle = characteristic.handle # Do not raise KeyError if notifications are not enabled on this characteristic # to be consistent with the behavior of the BlueZ backend. The entry is popped # up front so a second concurrent stop_notify on the same handle is the same # no-op it has always been instead of issuing a duplicate CCCD write and a # duplicate release; it is put back only when the release fails and is # therefore worth retrying. - if not (notify_cancel := self._notify_cancels.pop(characteristic.handle, None)): + if not (notify_cancel := self._notify_cancels.pop(handle, None)): + # The proxy-side subscription is already gone, but an earlier + # stop_notify may have failed to clear the CCCD. Retry that write + # instead of returning a success while the peripheral is still + # notifying. + if handle in self._cccd_dirty: + await self._async_clear_cccd(characteristic) return notify_stop, _ = notify_cancel try: - # Mirror of the connection v3 branch in start_notify: the esp32 - # never resolved the descriptors, so it cannot undo the CCCD - # write either. Without this the proxy stops forwarding the - # notifications but the peripheral keeps sending them for the - # life of the connection. Write the CCCD first so the peripheral - # is quiet before the proxy-side subscription goes away. - if self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value: - cccd_descriptor = characteristic.get_descriptor(CCCD_UUID) - if not cccd_descriptor: - # Same error start_notify raises for this condition: - # returning successfully here would hide a peripheral that - # keeps notifying for the life of the connection. - raise BleakError( - f"{self._description}: Characteristic {characteristic.uuid} " - "does not have a characteristic client config descriptor." - ) - _LOGGER.debug( - "%s: Writing to CCD descriptor %s to stop notifications", - self._description, - cccd_descriptor.handle, - ) - await self._client.bluetooth_gatt_write_descriptor( - self._address_as_int, - cccd_descriptor.handle, - CCCD_DISABLE_BYTES, - GATT_NOTIFY_TIMEOUT, - ) + # Write the CCCD first so the peripheral is quiet before the + # proxy-side subscription goes away. + await self._async_clear_cccd(characteristic) except BaseException as err: # Use BaseException to handle CancelledError as well as Exception. # Always release the proxy-side subscription, even when the CCCD # write fails. The release is best-effort here because the CCCD # failure is the actionable root cause and must not be replaced # by a secondary error from the cleanup path -- except for a - # CancelledError, which is a request to stop and has to win over - # any error it interrupts, so it is re-raised as-is below. + # BaseException that is not an Exception, which is a request to + # stop and has to win over any error it interrupts, so it is + # re-raised as-is below. try: await notify_stop() except BaseException as release_err: @@ -1052,24 +1043,22 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: # note the failure on the error the caller sees: while the # entry is retained it also blocks start_notify on this # handle, which is not something the CCCD error conveys. - retained = self._restore_notify_cancel( - characteristic.handle, notify_cancel - ) + retained = self._restore_notify_cancel(handle, notify_cancel) _LOGGER.warning( "%s: Failed to release the proxy notify subscription for " "handle %s; the proxy may keep forwarding notifications", self._description, - characteristic.handle, + handle, exc_info=True, ) - if isinstance(release_err, asyncio.CancelledError): - # A cancellation is a request to stop, so it wins over - # the CCCD error it interrupted instead of being demoted - # to a note on it. + if not isinstance(release_err, Exception): + # Cancellation, Ctrl-C and SystemExit are requests to + # stop; they win over the CCCD error they interrupted + # instead of being demoted to a note on it. raise err.add_note( "Releasing the proxy notify subscription for handle " - f"{characteristic.handle} also failed with {release_err!r}; " + f"{handle} also failed with {release_err!r}; " "the proxy may keep forwarding notifications." + ( " start_notify on this handle stays blocked until a" @@ -1085,9 +1074,55 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: except BaseException: # A failing release is retryable, so put the entry back rather # than leaving the handle unsubscribed in our bookkeeping only. - self._restore_notify_cancel(characteristic.handle, notify_cancel) + self._restore_notify_cancel(handle, notify_cancel) raise + async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> None: + """ + Write ``0x0000`` to the client config descriptor. + + Mirror of the connection v3 branch in ``start_notify``: the esp32 + never resolved the descriptors, so it cannot undo the CCCD write + either. Without this the proxy stops forwarding the notifications + but the peripheral keeps sending them for the life of the + connection. Nothing to do on firmware that resolves the descriptors + itself. + + The handle is recorded in ``_cccd_dirty`` while the write is + outstanding and forgotten again once it lands, so a ``stop_notify`` + that runs after the proxy-side subscription was already released can + tell a peripheral left notifying apart from a handle that was never + subscribed. + """ + if not self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value: + return + try: + cccd_descriptor = characteristic.get_descriptor(CCCD_UUID) + if not cccd_descriptor: + # Same error start_notify raises for this condition: + # returning successfully here would hide a peripheral that + # keeps notifying for the life of the connection. + raise BleakError( + f"{self._description}: Characteristic {characteristic.uuid} " + "does not have a characteristic client config descriptor." + ) + _LOGGER.debug( + "%s: Writing to CCD descriptor %s to stop notifications", + self._description, + cccd_descriptor.handle, + ) + await self._client.bluetooth_gatt_write_descriptor( + self._address_as_int, + cccd_descriptor.handle, + CCCD_DISABLE_BYTES, + GATT_NOTIFY_TIMEOUT, + ) + except BaseException: + # Use BaseException to handle CancelledError as well as Exception. + self._cccd_dirty.add(characteristic.handle) + raise + self._cccd_dirty.discard(characteristic.handle) + def _restore_notify_cancel( self, handle: int, @@ -1102,19 +1137,32 @@ def _restore_notify_cancel( ``start_notify`` on that handle for the life of the client. The abort half is run instead, mirroring what the cleanup would have done had the entry still been in the dict. ``setdefault`` keeps a - subscription made after the pop from being clobbered. + subscription made after the pop from being clobbered; the popped + entry has lost the handle in that case and is aborted too, so it is + not left dangling. Returns ------- - ``True`` when the entry is now in the dict, which is also when - it blocks ``start_notify`` on that handle. + ``True`` when the popped entry is the one now in the dict, which + is also when it blocks ``start_notify`` on that handle. """ + _, notify_abort = notify_cancel if not self._is_connected: - _, notify_abort = notify_cancel notify_abort() return False - self._notify_cancels.setdefault(handle, notify_cancel) + if self._notify_cancels.setdefault(handle, notify_cancel) is not notify_cancel: + # A start_notify after the pop already owns this handle, so the + # popped entry is stale: nothing will retry its release, and it + # must not be reported as retained. + _LOGGER.debug( + "%s: Notifications were re-enabled on handle %s while the " + "release was failing; discarding the stale entry", + self._description, + handle, + ) + notify_abort() + return False return True def _raise_if_not_connected(self) -> None: diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index c8056d5..5acf523 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -693,10 +693,14 @@ async def _resubscribe() -> None: char = Mock() char.handle = 99 char.uuid = "00002a05-0000-1000-8000-00805f9b34fb" - client._notify_cancels[99] = (_resubscribe, Mock()) + abort = Mock() + client._notify_cancels[99] = (_resubscribe, abort) with pytest.raises(RuntimeError): await client.stop_notify(char) assert client._notify_cancels[99] is newer + # The popped entry lost the handle, so it is aborted rather than left + # dangling with nothing to retry its release. + abort.assert_called_once() @pytest.mark.asyncio @@ -1045,3 +1049,65 @@ async def test_del_handles_loop_closed_race( side_effect=RuntimeError("loop closed"), ): client.__del__() # must not raise + + +@pytest.mark.asyncio +async def test_stop_notify_retries_cccd_after_a_failed_clear( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A later stop_notify re-attempts a CCCD clear that failed before.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + cccd = char.get_descriptor(CCCD_UUID) + assert cccd is not None + stop = AsyncMock() + client._notify_cancels[char.handle] = (stop, Mock()) + with ( + patch.object( + client._client, + "bluetooth_gatt_write_descriptor", + side_effect=BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)), + ), + pytest.raises(BleakError), + ): + await client.stop_notify(char) + assert char.handle not in client._notify_cancels + assert char.handle in client._cccd_dirty + # The proxy-side subscription is gone, but the peripheral is still + # notifying, so the retry must write the CCCD instead of returning. + with patch.object( + client._client, "bluetooth_gatt_write_descriptor" + ) as mock_write_desc: + await client.stop_notify(char) + mock_write_desc.assert_awaited_once_with( + client._address_as_int, cccd.handle, b"\x00\x00", GATT_NOTIFY_TIMEOUT + ) + assert char.handle not in client._cccd_dirty + stop.assert_awaited_once() + # Once the CCCD is clear the handle is a plain no-op again. + with patch.object( + client._client, "bluetooth_gatt_write_descriptor" + ) as mock_write_desc: + await client.stop_notify(char) + mock_write_desc.assert_not_called() + + +@pytest.mark.asyncio +async def test_disconnect_forgets_the_outstanding_cccd_clear( + client_data: ESPHomeClientData, +) -> None: + """A disconnect drops the pending CCCD write with the link.""" + client = _make_client(client_data) + client._is_connected = True + client._cccd_dirty.add(99) + client._async_disconnected_cleanup() + assert not client._cccd_dirty From 4106745ff2129fb55100a39540edbd078d4f7dc5 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Wed, 19 Aug 2026 17:02:36 +0000 Subject: [PATCH 12/18] fix: only mark a CCCD clear retryable when a retry can succeed --- src/bleak_esphome/backend/client.py | 35 +++++++++++++------ tests/backend/test_client_branches.py | 48 +++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 11 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 1c08b57..27f7473 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -1092,20 +1092,24 @@ async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> No outstanding and forgotten again once it lands, so a ``stop_notify`` that runs after the proxy-side subscription was already released can tell a peripheral left notifying apart from a handle that was never - subscribed. + subscribed. Only a failure that is actually retryable is recorded: a + characteristic with no CCCD at all, and a failure once the link is + already gone, leave the set untouched. """ if not self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value: return + cccd_descriptor = characteristic.get_descriptor(CCCD_UUID) + if not cccd_descriptor: + # Same error start_notify raises for this condition: returning + # successfully here would hide a peripheral that keeps notifying + # for the life of the connection. Raised before the try below so + # the handle is not marked dirty: there is no descriptor to write, + # so there is nothing a later stop_notify could retry. + raise BleakError( + f"{self._description}: Characteristic {characteristic.uuid} " + "does not have a characteristic client config descriptor." + ) try: - cccd_descriptor = characteristic.get_descriptor(CCCD_UUID) - if not cccd_descriptor: - # Same error start_notify raises for this condition: - # returning successfully here would hide a peripheral that - # keeps notifying for the life of the connection. - raise BleakError( - f"{self._description}: Characteristic {characteristic.uuid} " - "does not have a characteristic client config descriptor." - ) _LOGGER.debug( "%s: Writing to CCD descriptor %s to stop notifications", self._description, @@ -1119,7 +1123,16 @@ async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> No ) except BaseException: # Use BaseException to handle CancelledError as well as Exception. - self._cccd_dirty.add(characteristic.handle) + # Only record the handle while the link is up, mirroring the guard + # _restore_notify_cancel applies to the notify entry: the + # disconnect cleanup may have run inside the await and already + # cleared the set, and a TimeoutAPIError or CancelledError does not + # trigger a second cleanup, so an unguarded add would survive the + # disconnect and turn the next connection's no-op stop_notify into + # a real CCCD round trip. A disconnect stops the notifications + # anyway, so there is nothing left to retry. + if self._is_connected: + self._cccd_dirty.add(characteristic.handle) raise self._cccd_dirty.discard(characteristic.handle) diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index 5acf523..bef1561 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -598,6 +598,15 @@ async def test_stop_notify_raises_when_cccd_missing( mock_write_desc.assert_not_called() stop.assert_awaited_once() assert 99 not in client._notify_cancels + # There is no descriptor to write, so nothing is retryable: the handle + # must not be latched as dirty or every later stop_notify would raise + # instead of returning the BlueZ-compatible no-op. + assert 99 not in client._cccd_dirty + with patch.object( + client._client, "bluetooth_gatt_write_descriptor" + ) as mock_write_desc: + await client.stop_notify(char) + mock_write_desc.assert_not_called() @pytest.mark.asyncio @@ -674,6 +683,45 @@ async def _disconnect_and_raise(*args: Any, **kwargs: Any) -> None: stop.assert_awaited_once() abort.assert_called_once() assert char.handle not in client._notify_cancels + # The cleanup ran inside the write, so recording the handle afterwards + # would leave it dirty for the life of the next connection. + assert not client._cccd_dirty + + +@pytest.mark.asyncio +async def test_stop_notify_forgets_cccd_when_disconnected_mid_write( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A timeout after a disconnect must not latch the handle as dirty.""" + client = _make_client(client_data) + client._is_connected = True + with patch.object( + client._client, + "bluetooth_gatt_get_services", + return_value=esphome_bluetooth_gatt_services, + ): + services = await client._get_services() + char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") + assert char is not None + client._notify_cancels[char.handle] = (AsyncMock(), Mock()) + + async def _disconnect_and_time_out(*args: Any, **kwargs: Any) -> None: + client._async_disconnected_cleanup() + raise TimeoutAPIError("timed out") + + # A TimeoutAPIError does not trigger a second disconnected cleanup, so an + # unguarded add would survive the disconnect. + with ( + patch.object( + client._client, + "bluetooth_gatt_write_descriptor", + side_effect=_disconnect_and_time_out, + ), + pytest.raises(TimeoutError), + ): + await client.stop_notify(char) + assert not client._cccd_dirty @pytest.mark.asyncio From 9b8767b4af8768179bcb5190cafab144e3314d31 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Wed, 19 Aug 2026 17:42:12 +0000 Subject: [PATCH 13/18] refactor: extract the cccd write and notify release helpers in stop_notify --- src/bleak_esphome/backend/client.py | 252 ++++++++++++-------------- tests/backend/test_client_branches.py | 8 +- 2 files changed, 122 insertions(+), 138 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 27f7473..1582277 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -74,21 +74,6 @@ _ESPHomeClient = TypeVar("_ESPHomeClient", bound="ESPHomeClient") _R = TypeVar("_R") _P = ParamSpec("_P") -_E = TypeVar("_E", bound=BaseException) - - -def _with_notes(err: _E, cause: BaseException) -> _E: - """ - Carry any notes on ``cause`` over to the error the caller sees. - - The api errors below are re-raised as bleak errors, so a note attached - to the original -- for example ``stop_notify``'s failed-release detail - -- would otherwise only be reachable through ``__cause__`` and be - absent from the traceback of the error the caller actually catches. - """ - for note in getattr(cause, "__notes__", ()): - err.add_note(note) - return err def api_error_as_bleak_error( @@ -104,7 +89,7 @@ async def _async_wrap_bluetooth_operation( try: return await func(self, *args, **kwargs) except TimeoutAPIError as err: - raise _with_notes(TimeoutError(str(err)), err) from err + raise TimeoutError(str(err)) from err except BluetoothConnectionDroppedError as ex: _LOGGER.debug( "%s: BLE device disconnected during %s operation", @@ -112,7 +97,7 @@ async def _async_wrap_bluetooth_operation( func.__name__, ) self._async_ble_device_disconnected() - raise _with_notes(BleakError(str(ex)), ex) from ex + raise BleakError(str(ex)) from ex except BluetoothGATTAPIError as ex: # If the device disconnects in the middle of an operation # be sure to mark it as disconnected so any library using @@ -129,9 +114,9 @@ async def _async_wrap_bluetooth_operation( func.__name__, ) self._async_ble_device_disconnected() - raise _with_notes(BleakError(str(ex)), ex) from ex + raise BleakError(str(ex)) from ex except APIConnectionError as err: - raise _with_notes(BleakError(str(err)), err) from err + raise BleakError(str(err)) from err return _async_wrap_bluetooth_operation @@ -929,32 +914,17 @@ async def start_notify( ) ) - if not self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value: - return - try: # For connection v3 we are responsible for enabling notifications # on the cccd (characteristic client config descriptor) handle since # the esp32 will not have resolved the characteristic descriptors to # save memory since doing so can exhaust the memory and cause a soft # reset - cccd_descriptor = characteristic.get_descriptor(CCCD_UUID) - if not cccd_descriptor: - raise BleakError( - f"{self._description}: Characteristic {characteristic.uuid} " - "does not have a characteristic client config descriptor." - ) - - _LOGGER.debug( - "%s: Writing to CCD descriptor %s for notifications with properties=%s", - self._description, - cccd_descriptor.handle, - characteristic.properties, - ) + if (cccd_descriptor := self._get_cccd(characteristic)) is None: + return supports_notify = "notify" in characteristic.properties - await self._client.bluetooth_gatt_write_descriptor( - self._address_as_int, - cccd_descriptor.handle, + await self._async_write_cccd( + cccd_descriptor, CCCD_NOTIFY_BYTES if supports_notify else CCCD_INDICATE_BYTES, timeout, ) @@ -985,17 +955,15 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: ``BleakError`` where it previously always returned. Both failures are retryable. A failed proxy-side release keeps the subscription entry so a later ``stop_notify`` retries it; while it is retained it - also blocks ``start_notify`` on that handle, which shares the same - bookkeeping as its duplicate-subscription guard, so the release - failure is attached as a note to the error the caller receives and - the two can be told apart. A failed CCCD write releases the - proxy-side subscription and drops the entry, but the handle is - remembered so a later ``stop_notify`` re-attempts the descriptor - write instead of returning a success while the peripheral keeps - notifying. A release that fails because the device disconnected - drops the entry instead of retaining it, so a reconnect can subscribe - to that handle again; a disconnect likewise forgets any outstanding - CCCD write, since the peripheral stops notifying with the link. + also blocks ``start_notify`` on that handle. A failed CCCD write + releases the proxy-side subscription and drops the entry, but the + handle is remembered so a later ``stop_notify`` re-attempts the + descriptor write instead of returning a success while the peripheral + keeps notifying. A release that fails because the device + disconnected drops the entry instead of retaining it, so a reconnect + can subscribe to that handle again; a disconnect likewise forgets any + outstanding CCCD write, since the peripheral stops notifying with the + link. Args: ---- @@ -1021,62 +989,107 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: if handle in self._cccd_dirty: await self._async_clear_cccd(characteristic) return - notify_stop, _ = notify_cancel try: # Write the CCCD first so the peripheral is quiet before the # proxy-side subscription goes away. await self._async_clear_cccd(characteristic) - except BaseException as err: + except BaseException: # Use BaseException to handle CancelledError as well as Exception. # Always release the proxy-side subscription, even when the CCCD - # write fails. The release is best-effort here because the CCCD - # failure is the actionable root cause and must not be replaced - # by a secondary error from the cleanup path -- except for a - # BaseException that is not an Exception, which is a request to - # stop and has to win over any error it interrupts, so it is - # re-raised as-is below. - try: - await notify_stop() - except BaseException as release_err: - # Put the entry back so a later stop_notify retries the - # release instead of hitting the missing-handle no-op, and - # note the failure on the error the caller sees: while the - # entry is retained it also blocks start_notify on this - # handle, which is not something the CCCD error conveys. - retained = self._restore_notify_cancel(handle, notify_cancel) - _LOGGER.warning( - "%s: Failed to release the proxy notify subscription for " - "handle %s; the proxy may keep forwarding notifications", - self._description, - handle, - exc_info=True, - ) - if not isinstance(release_err, Exception): - # Cancellation, Ctrl-C and SystemExit are requests to - # stop; they win over the CCCD error they interrupted - # instead of being demoted to a note on it. - raise - err.add_note( - "Releasing the proxy notify subscription for handle " - f"{handle} also failed with {release_err!r}; " - "the proxy may keep forwarding notifications." - + ( - " start_notify on this handle stays blocked until a" - " later stop_notify succeeds or the device" - " disconnects." - if retained - else "" - ) - ) + # write fails; that failure is the actionable root cause and must + # not be replaced by a secondary error from the cleanup path. + await self._async_release_notify(handle, notify_cancel, best_effort=True) raise + await self._async_release_notify(handle, notify_cancel) + + async def _async_release_notify( + self, + handle: int, + notify_cancel: _NotifyCancel, + best_effort: bool = False, + ) -> None: + """ + Release the proxy-side notify subscription for ``handle``. + + A failing release is retryable, so the popped entry is put back + rather than leaving the handle unsubscribed in our bookkeeping only. + With ``best_effort`` the caller is already unwinding a failed CCCD + write, so an ordinary release failure is logged instead of replacing + that error; a ``BaseException`` that is not an ``Exception`` is a + request to stop and still wins over the error it interrupted. + """ + notify_stop, _ = notify_cancel try: await notify_stop() - except BaseException: - # A failing release is retryable, so put the entry back rather - # than leaving the handle unsubscribed in our bookkeeping only. + except BaseException as release_err: + # Use BaseException to handle CancelledError as well as Exception. self._restore_notify_cancel(handle, notify_cancel) + if not best_effort: + raise + _LOGGER.warning( + "%s: Failed to release the proxy notify subscription for " + "handle %s; the proxy may keep forwarding notifications", + self._description, + handle, + exc_info=True, + ) + if isinstance(release_err, Exception): + return + # Cancellation, Ctrl-C and SystemExit are requests to stop, so they + # win over the CCCD failure they interrupted. That failure never + # reaches the caller, so log it here instead of dropping it. + _LOGGER.warning( + "%s: Clearing the CCCD for handle %s failed; the peripheral " + "may keep notifying", + self._description, + handle, + exc_info=release_err.__context__, + ) raise + def _get_cccd( + self, characteristic: BleakGATTCharacteristic + ) -> BleakGATTDescriptor | None: + """ + Return the client config descriptor this host has to write itself. + + ``None`` on firmware without ``REMOTE_CACHING``: the esp32 resolved + the descriptors and drives the CCCD itself, so neither + ``start_notify`` nor ``stop_notify`` touches it. On connection v3 the + esp32 skipped that resolution to save memory, so a characteristic + without a CCCD is an error: subscribing would never reach the + peripheral, and unsubscribing would leave it notifying for the life + of the connection. + """ + if not self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value: + return None + if cccd_descriptor := characteristic.get_descriptor(CCCD_UUID): + return cccd_descriptor + raise BleakError( + f"{self._description}: Characteristic {characteristic.uuid} " + "does not have a characteristic client config descriptor." + ) + + async def _async_write_cccd( + self, + cccd_descriptor: BleakGATTDescriptor, + value: bytes, + timeout: float, + ) -> None: + """Write ``value`` to a client config descriptor.""" + _LOGGER.debug( + "%s: Writing %s to CCD descriptor %s", + self._description, + value.hex(), + cccd_descriptor.handle, + ) + await self._client.bluetooth_gatt_write_descriptor( + self._address_as_int, + cccd_descriptor.handle, + value, + timeout, + ) + async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> None: """ Write ``0x0000`` to the client config descriptor. @@ -1085,41 +1098,23 @@ async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> No never resolved the descriptors, so it cannot undo the CCCD write either. Without this the proxy stops forwarding the notifications but the peripheral keeps sending them for the life of the - connection. Nothing to do on firmware that resolves the descriptors - itself. + connection. The handle is recorded in ``_cccd_dirty`` while the write is outstanding and forgotten again once it lands, so a ``stop_notify`` that runs after the proxy-side subscription was already released can tell a peripheral left notifying apart from a handle that was never subscribed. Only a failure that is actually retryable is recorded: a - characteristic with no CCCD at all, and a failure once the link is - already gone, leave the set untouched. + characteristic with no CCCD at all raises before anything is + recorded, and a failure once the link is already gone leaves the set + untouched. """ - if not self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value: + if (cccd_descriptor := self._get_cccd(characteristic)) is None: return - cccd_descriptor = characteristic.get_descriptor(CCCD_UUID) - if not cccd_descriptor: - # Same error start_notify raises for this condition: returning - # successfully here would hide a peripheral that keeps notifying - # for the life of the connection. Raised before the try below so - # the handle is not marked dirty: there is no descriptor to write, - # so there is nothing a later stop_notify could retry. - raise BleakError( - f"{self._description}: Characteristic {characteristic.uuid} " - "does not have a characteristic client config descriptor." - ) + handle = characteristic.handle try: - _LOGGER.debug( - "%s: Writing to CCD descriptor %s to stop notifications", - self._description, - cccd_descriptor.handle, - ) - await self._client.bluetooth_gatt_write_descriptor( - self._address_as_int, - cccd_descriptor.handle, - CCCD_DISABLE_BYTES, - GATT_NOTIFY_TIMEOUT, + await self._async_write_cccd( + cccd_descriptor, CCCD_DISABLE_BYTES, GATT_NOTIFY_TIMEOUT ) except BaseException: # Use BaseException to handle CancelledError as well as Exception. @@ -1132,15 +1127,15 @@ async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> No # a real CCCD round trip. A disconnect stops the notifications # anyway, so there is nothing left to retry. if self._is_connected: - self._cccd_dirty.add(characteristic.handle) + self._cccd_dirty.add(handle) raise - self._cccd_dirty.discard(characteristic.handle) + self._cccd_dirty.discard(handle) def _restore_notify_cancel( self, handle: int, notify_cancel: _NotifyCancel, - ) -> bool: + ) -> None: """ Put a popped notify entry back so a failed release stays retryable. @@ -1153,21 +1148,14 @@ def _restore_notify_cancel( subscription made after the pop from being clobbered; the popped entry has lost the handle in that case and is aborted too, so it is not left dangling. - - Returns - ------- - ``True`` when the popped entry is the one now in the dict, which - is also when it blocks ``start_notify`` on that handle. - """ _, notify_abort = notify_cancel if not self._is_connected: notify_abort() - return False + return if self._notify_cancels.setdefault(handle, notify_cancel) is not notify_cancel: # A start_notify after the pop already owns this handle, so the - # popped entry is stale: nothing will retry its release, and it - # must not be reported as retained. + # popped entry is stale: nothing will retry its release. _LOGGER.debug( "%s: Notifications were re-enabled on handle %s while the " "release was failing; discarding the stale entry", @@ -1175,8 +1163,6 @@ def _restore_notify_cancel( handle, ) notify_abort() - return False - return True def _raise_if_not_connected(self) -> None: """Raise a BleakError if not connected.""" diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index bef1561..fa6435d 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -635,16 +635,14 @@ async def test_stop_notify_cccd_failure_survives_failing_release( "bluetooth_gatt_write_descriptor", side_effect=BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)), ), - pytest.raises(BleakError) as excinfo, + pytest.raises(BleakError), ): await client.stop_notify(char) stop.assert_awaited_once() assert char.handle in client._notify_cancels + # The CCCD failure is the actionable root cause the caller gets; the + # secondary release failure is reported through the log. assert "Failed to release the proxy notify subscription" in caplog.text - # The note has to be on the error the caller actually catches, not only - # on the api error the decorator wrapped. - notes = getattr(excinfo.value, "__notes__", []) - assert any("also failed with" in note for note in notes) @pytest.mark.asyncio From e415b85cf5748fcf76de24c3708a3db0be63e5b6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 19 Aug 2026 20:42:48 -0500 Subject: [PATCH 14/18] refactor: deduplicate stop_notify test setup and trim redundant prose --- src/bleak_esphome/backend/client.py | 24 +--- tests/backend/_helpers.py | 2 + tests/backend/test_client.py | 2 +- tests/backend/test_client_branches.py | 198 ++++++++------------------ 4 files changed, 71 insertions(+), 155 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 1582277..3a624de 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -953,17 +953,10 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: The CCCD write is a round trip to the peripheral, so this method can block for up to the proxy GATT timeout and can raise ``BleakError`` where it previously always returned. Both failures - are retryable. A failed proxy-side release keeps the subscription - entry so a later ``stop_notify`` retries it; while it is retained it - also blocks ``start_notify`` on that handle. A failed CCCD write - releases the proxy-side subscription and drops the entry, but the - handle is remembered so a later ``stop_notify`` re-attempts the - descriptor write instead of returning a success while the peripheral - keeps notifying. A release that fails because the device - disconnected drops the entry instead of retaining it, so a reconnect - can subscribe to that handle again; a disconnect likewise forgets any - outstanding CCCD write, since the peripheral stops notifying with the - link. + are retryable by calling ``stop_notify`` again; which bookkeeping + survives each failure — and how a disconnect discards all of it — + is documented on ``_async_release_notify``, ``_async_clear_cccd`` + and ``_restore_notify_cancel``. Args: ---- @@ -1022,7 +1015,6 @@ async def _async_release_notify( try: await notify_stop() except BaseException as release_err: - # Use BaseException to handle CancelledError as well as Exception. self._restore_notify_cancel(handle, notify_cancel) if not best_effort: raise @@ -1080,7 +1072,7 @@ async def _async_write_cccd( _LOGGER.debug( "%s: Writing %s to CCD descriptor %s", self._description, - value.hex(), + value, cccd_descriptor.handle, ) await self._client.bluetooth_gatt_write_descriptor( @@ -1104,10 +1096,7 @@ async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> No outstanding and forgotten again once it lands, so a ``stop_notify`` that runs after the proxy-side subscription was already released can tell a peripheral left notifying apart from a handle that was never - subscribed. Only a failure that is actually retryable is recorded: a - characteristic with no CCCD at all raises before anything is - recorded, and a failure once the link is already gone leaves the set - untouched. + subscribed. """ if (cccd_descriptor := self._get_cccd(characteristic)) is None: return @@ -1117,7 +1106,6 @@ async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> No cccd_descriptor, CCCD_DISABLE_BYTES, GATT_NOTIFY_TIMEOUT ) except BaseException: - # Use BaseException to handle CancelledError as well as Exception. # Only record the handle while the link is up, mirroring the guard # _restore_notify_cancel applies to the notify entry: the # disconnect cleanup may have run inside the await and already diff --git a/tests/backend/_helpers.py b/tests/backend/_helpers.py index 6dd7539..0dfcf54 100644 --- a/tests/backend/_helpers.py +++ b/tests/backend/_helpers.py @@ -23,6 +23,8 @@ ESP_MAC_ADDRESS = "AA:BB:CC:DD:EE:FF" ESP_NAME = "proxy" BLE_ADDRESS = "CC:BB:AA:DD:EE:FF" +# The indicate-only characteristic in the standard GATT services fixture. +INDICATE_CHAR_UUID = "00002a05-0000-1000-8000-00805f9b34fb" def make_ble_device() -> BLEDevice: diff --git a/tests/backend/test_client.py b/tests/backend/test_client.py index 2110363..dc26e04 100644 --- a/tests/backend/test_client.py +++ b/tests/backend/test_client.py @@ -30,6 +30,7 @@ from ._helpers import ( ESP_MAC_ADDRESS, ESP_NAME, + INDICATE_CHAR_UUID, fetch_services, make_bleak_client, patch_connect_rpcs, @@ -38,7 +39,6 @@ ) PRIMARY_CHAR_UUID = "090b7847-e12b-09a8-b04b-8e0922a9abab" -INDICATE_CHAR_UUID = "00002a05-0000-1000-8000-00805f9b34fb" CCCD_UUID = "00002902-0000-1000-8000-00805f9b34fb" BLE_ADDRESS_AS_INT = 225106397622015 diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index fa6435d..5b891e0 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -37,13 +37,32 @@ from pytest_asyncio import fixture as aio_fixture from bleak_esphome.backend.client import ( + CCCD_DISABLE_BYTES, CCCD_UUID, GATT_NOTIFY_TIMEOUT, ESPHomeClient, ESPHomeClientData, ) -from ._helpers import ESP_MAC_ADDRESS, _make_client +from ._helpers import ( + ESP_MAC_ADDRESS, + INDICATE_CHAR_UUID, + _make_client, + fetch_services, +) + + +async def _connected_client_with_char( + client_data: ESPHomeClientData, + services_payload: ESPHomeBluetoothGATTServices, +) -> tuple[ESPHomeClient, BleakGATTCharacteristic]: + """Return a connected client and the fixture's indicate characteristic.""" + client = _make_client(client_data) + client._is_connected = True + services = await fetch_services(client, services_payload) + char = services.get_characteristic(INDICATE_CHAR_UUID) + assert char is not None + return client, char @pytest.mark.asyncio @@ -394,16 +413,9 @@ async def test_start_notify_already_enabled_raises( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """A second start_notify on the same handle raises BleakError.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) # Pre-populate the cancels dict; the implementation only inspects keys. client._notify_cancels[char.handle] = (AsyncMock(), Mock()) with pytest.raises(BleakError, match="already enabled"): @@ -439,16 +451,9 @@ async def test_start_notify_skips_cccd_without_remote_caching( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """Without REMOTE_CACHING the host does not write to the CCCD itself.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) # Disable REMOTE_CACHING so start_notify returns before touching the CCCD. client._feature_flags &= ~BluetoothProxyFeature.REMOTE_CACHING.value mock_stop_notify = AsyncMock() @@ -492,16 +497,9 @@ async def test_stop_notify_disables_cccd( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """With REMOTE_CACHING the host clears the CCCD it wrote on start.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) cccd = char.get_descriptor(CCCD_UUID) assert cccd is not None stop = AsyncMock() @@ -511,7 +509,7 @@ async def test_stop_notify_disables_cccd( ) as mock_write_desc: await client.stop_notify(char) mock_write_desc.assert_awaited_once_with( - client._address_as_int, cccd.handle, b"\x00\x00", GATT_NOTIFY_TIMEOUT + client._address_as_int, cccd.handle, CCCD_DISABLE_BYTES, GATT_NOTIFY_TIMEOUT ) stop.assert_awaited_once() assert char.handle not in client._notify_cancels @@ -523,16 +521,9 @@ async def test_stop_notify_skips_cccd_without_remote_caching( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """Without REMOTE_CACHING the esp32 owns the CCCD, so the host leaves it.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) client._feature_flags &= ~BluetoothProxyFeature.REMOTE_CACHING.value stop = AsyncMock() client._notify_cancels[char.handle] = (stop, Mock()) @@ -550,16 +541,9 @@ async def test_stop_notify_releases_proxy_when_cccd_write_fails( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """A failing CCCD write still releases the proxy-side subscription.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) stop = AsyncMock() client._notify_cancels[char.handle] = (stop, Mock()) with ( @@ -585,7 +569,7 @@ async def test_stop_notify_raises_when_cccd_missing( stop = AsyncMock() char = Mock() char.handle = 99 - char.uuid = "00002a05-0000-1000-8000-00805f9b34fb" + char.uuid = INDICATE_CHAR_UUID char.get_descriptor.return_value = None client._notify_cancels[99] = (stop, Mock()) with ( @@ -616,16 +600,9 @@ async def test_stop_notify_cccd_failure_survives_failing_release( caplog: pytest.LogCaptureFixture, ) -> None: """A failing release keeps the entry so the caller can retry it.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) stop = AsyncMock(side_effect=RuntimeError("release failed")) client._notify_cancels[char.handle] = (stop, Mock()) with ( @@ -651,16 +628,9 @@ async def test_stop_notify_drops_entry_when_release_fails_after_disconnect( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """A disconnect during the CCCD write must not resurrect the entry.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) stop = AsyncMock(side_effect=RuntimeError("release failed")) abort = Mock() client._notify_cancels[char.handle] = (stop, abort) @@ -692,16 +662,9 @@ async def test_stop_notify_forgets_cccd_when_disconnected_mid_write( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """A timeout after a disconnect must not latch the handle as dirty.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) client._notify_cancels[char.handle] = (AsyncMock(), Mock()) async def _disconnect_and_time_out(*args: Any, **kwargs: Any) -> None: @@ -738,7 +701,6 @@ async def _resubscribe() -> None: char = Mock() char.handle = 99 - char.uuid = "00002a05-0000-1000-8000-00805f9b34fb" abort = Mock() client._notify_cancels[99] = (_resubscribe, abort) with pytest.raises(RuntimeError): @@ -755,16 +717,9 @@ async def test_stop_notify_cccd_failure_survives_cancelled_release( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """A cancelled release propagates instead of being demoted to a log.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) stop = AsyncMock(side_effect=asyncio.CancelledError()) client._notify_cancels[char.handle] = (stop, Mock()) with ( @@ -791,7 +746,6 @@ async def test_stop_notify_keeps_entry_when_release_fails( stop = AsyncMock(side_effect=RuntimeError("release failed")) char = Mock() char.handle = 99 - char.uuid = "00002a05-0000-1000-8000-00805f9b34fb" client._notify_cancels[99] = (stop, Mock()) with pytest.raises(RuntimeError): await client.stop_notify(char) @@ -805,16 +759,9 @@ async def test_stop_notify_survives_concurrent_cancel_clear( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """A disconnect clearing the dict during the CCCD write is not a KeyError.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) stop = AsyncMock() client._notify_cancels[char.handle] = (stop, Mock()) @@ -835,16 +782,9 @@ async def test_stop_notify_is_single_winner_when_reentered( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """A second stop_notify during the CCCD write is a no-op, not a duplicate.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) stop = AsyncMock() client._notify_cancels[char.handle] = (stop, Mock()) @@ -866,16 +806,9 @@ async def test_stop_notify_cccd_failure_survives_concurrent_cancel_clear( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """The error path pops defensively when a disconnect cleared the dict.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) async def _clear_and_raise(*args: Any, **kwargs: Any) -> None: client._notify_cancels.clear() @@ -1103,16 +1036,9 @@ async def test_stop_notify_retries_cccd_after_a_failed_clear( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, ) -> None: """A later stop_notify re-attempts a CCCD clear that failed before.""" - client = _make_client(client_data) - client._is_connected = True - with patch.object( - client._client, - "bluetooth_gatt_get_services", - return_value=esphome_bluetooth_gatt_services, - ): - services = await client._get_services() - char = services.get_characteristic("00002a05-0000-1000-8000-00805f9b34fb") - assert char is not None + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) cccd = char.get_descriptor(CCCD_UUID) assert cccd is not None stop = AsyncMock() @@ -1135,7 +1061,7 @@ async def test_stop_notify_retries_cccd_after_a_failed_clear( ) as mock_write_desc: await client.stop_notify(char) mock_write_desc.assert_awaited_once_with( - client._address_as_int, cccd.handle, b"\x00\x00", GATT_NOTIFY_TIMEOUT + client._address_as_int, cccd.handle, CCCD_DISABLE_BYTES, GATT_NOTIFY_TIMEOUT ) assert char.handle not in client._cccd_dirty stop.assert_awaited_once() From 481ec0ccc453c3247f83ceb3ce3693abdea9bbca Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Thu, 20 Aug 2026 03:42:38 +0000 Subject: [PATCH 15/18] refactor: drop the notify release retry machinery and track dirty CCCD writes --- src/bleak_esphome/backend/client.py | 203 +++++++++----------------- tests/backend/test_client_branches.py | 133 ++++++++--------- 2 files changed, 138 insertions(+), 198 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 3a624de..e541a2b 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -169,8 +169,8 @@ def __init__( self._mtu: int | None = None self._cancel_connection_state: Callable[[], None] | None = None self._notify_cancels: dict[int, _NotifyCancel] = {} - # Handles whose CCCD clear failed after the proxy-side subscription - # was already released, so a later stop_notify retries the write + # Handles whose CCCD write may have landed while the local + # bookkeeping was unwound, so a later stop_notify retries the clear # instead of returning the missing-handle no-op. self._cccd_dirty: set[int] = set() self._device_info = client_data.device_info @@ -924,6 +924,7 @@ async def start_notify( return supports_notify = "notify" in characteristic.properties await self._async_write_cccd( + ble_handle, cccd_descriptor, CCCD_NOTIFY_BYTES if supports_notify else CCCD_INDICATE_BYTES, timeout, @@ -945,18 +946,10 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: need to track which characteristics they have subscribed to. On connection v3 (``REMOTE_CACHING``) proxies the client config - descriptor is cleared so the peripheral stops notifying. A missing - descriptor raises ``BleakError`` with the same message - ``start_notify`` uses, so a caller never gets a successful return - while the peripheral keeps notifying. - - The CCCD write is a round trip to the peripheral, so this method - can block for up to the proxy GATT timeout and can raise - ``BleakError`` where it previously always returned. Both failures - are retryable by calling ``stop_notify`` again; which bookkeeping - survives each failure — and how a disconnect discards all of it — - is documented on ``_async_release_notify``, ``_async_clear_cccd`` - and ``_restore_notify_cancel``. + descriptor is cleared so the peripheral stops notifying. That is a + round trip, so this method can block for up to the proxy GATT + timeout and can raise ``BleakError`` where it previously always + returned; calling ``stop_notify`` again retries the write. Args: ---- @@ -972,72 +965,39 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: # to be consistent with the behavior of the BlueZ backend. The entry is popped # up front so a second concurrent stop_notify on the same handle is the same # no-op it has always been instead of issuing a duplicate CCCD write and a - # duplicate release; it is put back only when the release fails and is - # therefore worth retrying. + # duplicate release. if not (notify_cancel := self._notify_cancels.pop(handle, None)): - # The proxy-side subscription is already gone, but an earlier - # stop_notify may have failed to clear the CCCD. Retry that write - # instead of returning a success while the peripheral is still - # notifying. + # The proxy-side subscription is already gone, but an earlier CCCD + # write may have failed. Retry it instead of returning a success + # while the peripheral is still notifying. if handle in self._cccd_dirty: await self._async_clear_cccd(characteristic) return + notify_stop, _ = notify_cancel try: # Write the CCCD first so the peripheral is quiet before the # proxy-side subscription goes away. await self._async_clear_cccd(characteristic) except BaseException: # Use BaseException to handle CancelledError as well as Exception. - # Always release the proxy-side subscription, even when the CCCD - # write fails; that failure is the actionable root cause and must - # not be replaced by a secondary error from the cleanup path. - await self._async_release_notify(handle, notify_cancel, best_effort=True) - raise - await self._async_release_notify(handle, notify_cancel) - - async def _async_release_notify( - self, - handle: int, - notify_cancel: _NotifyCancel, - best_effort: bool = False, - ) -> None: - """ - Release the proxy-side notify subscription for ``handle``. - - A failing release is retryable, so the popped entry is put back - rather than leaving the handle unsubscribed in our bookkeeping only. - With ``best_effort`` the caller is already unwinding a failed CCCD - write, so an ordinary release failure is logged instead of replacing - that error; a ``BaseException`` that is not an ``Exception`` is a - request to stop and still wins over the error it interrupted. - """ - notify_stop, _ = notify_cancel - try: - await notify_stop() - except BaseException as release_err: - self._restore_notify_cancel(handle, notify_cancel) - if not best_effort: - raise - _LOGGER.warning( - "%s: Failed to release the proxy notify subscription for " - "handle %s; the proxy may keep forwarding notifications", - self._description, - handle, - exc_info=True, - ) - if isinstance(release_err, Exception): - return - # Cancellation, Ctrl-C and SystemExit are requests to stop, so they - # win over the CCCD failure they interrupted. That failure never - # reaches the caller, so log it here instead of dropping it. - _LOGGER.warning( - "%s: Clearing the CCCD for handle %s failed; the peripheral " - "may keep notifying", - self._description, - handle, - exc_info=release_err.__context__, - ) + # Release the proxy-side subscription anyway, but let the CCCD + # failure reach the caller: it is the actionable root cause. + # aioesphomeapi's stop_notify is an idempotent, synchronous body in + # an async wrapper, so it only fails once the API connection is + # gone -- there is nothing to retry and the disconnect cleanup owns + # the rest. + try: + await notify_stop() + except Exception: + _LOGGER.warning( + "%s: Failed to release the proxy notify subscription for " + "handle %s; the proxy may keep forwarding notifications", + self._description, + handle, + exc_info=True, + ) raise + await notify_stop() def _get_cccd( self, characteristic: BleakGATTCharacteristic @@ -1064,23 +1024,48 @@ def _get_cccd( async def _async_write_cccd( self, + handle: int, cccd_descriptor: BleakGATTDescriptor, value: bytes, timeout: float, ) -> None: - """Write ``value`` to a client config descriptor.""" + """ + Write ``value`` to the client config descriptor of ``handle``. + + A failed write may still have landed on the peripheral, so the + characteristic handle is recorded in ``_cccd_dirty`` and a later + ``stop_notify`` clears the descriptor instead of taking the + missing-handle no-op. Only while the link is up: the disconnect + cleanup may have run inside the await and a ``TimeoutAPIError`` or + ``CancelledError`` does not trigger a second one, so an unguarded add + would survive into the next connection -- which has nothing left to + retry anyway. + """ _LOGGER.debug( "%s: Writing %s to CCD descriptor %s", self._description, value, cccd_descriptor.handle, ) - await self._client.bluetooth_gatt_write_descriptor( - self._address_as_int, - cccd_descriptor.handle, - value, - timeout, - ) + try: + await self._client.bluetooth_gatt_write_descriptor( + self._address_as_int, + cccd_descriptor.handle, + value, + timeout, + ) + except BaseException: + if self._is_connected: + self._cccd_dirty.add(handle) + raise + if handle in self._notify_cancels: + # A start_notify owns the handle: either this write is its own + # enable, or one raced a clear and has a write in flight against + # the same descriptor with no ordering guarantee. Leave the handle + # dirty so the next stop_notify clears it again instead of + # declaring a descriptor it may have lost the race for clean. + return + self._cccd_dirty.discard(handle) async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> None: """ @@ -1092,65 +1077,19 @@ async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> No but the peripheral keeps sending them for the life of the connection. - The handle is recorded in ``_cccd_dirty`` while the write is - outstanding and forgotten again once it lands, so a ``stop_notify`` - that runs after the proxy-side subscription was already released can - tell a peripheral left notifying apart from a handle that was never - subscribed. + ``_async_write_cccd`` tracks whether the write landed, so a + ``stop_notify`` that runs after the proxy-side subscription was + already released can tell a peripheral left notifying apart from a + handle that was never subscribed. """ if (cccd_descriptor := self._get_cccd(characteristic)) is None: return - handle = characteristic.handle - try: - await self._async_write_cccd( - cccd_descriptor, CCCD_DISABLE_BYTES, GATT_NOTIFY_TIMEOUT - ) - except BaseException: - # Only record the handle while the link is up, mirroring the guard - # _restore_notify_cancel applies to the notify entry: the - # disconnect cleanup may have run inside the await and already - # cleared the set, and a TimeoutAPIError or CancelledError does not - # trigger a second cleanup, so an unguarded add would survive the - # disconnect and turn the next connection's no-op stop_notify into - # a real CCCD round trip. A disconnect stops the notifications - # anyway, so there is nothing left to retry. - if self._is_connected: - self._cccd_dirty.add(handle) - raise - self._cccd_dirty.discard(handle) - - def _restore_notify_cancel( - self, - handle: int, - notify_cancel: _NotifyCancel, - ) -> None: - """ - Put a popped notify entry back so a failed release stays retryable. - - Nothing is restored once the device has disconnected: the popped - entry was invisible to ``_async_disconnected_cleanup``, so putting - it back would resurrect state nothing clears again and block - ``start_notify`` on that handle for the life of the client. The - abort half is run instead, mirroring what the cleanup would have - done had the entry still been in the dict. ``setdefault`` keeps a - subscription made after the pop from being clobbered; the popped - entry has lost the handle in that case and is aborted too, so it is - not left dangling. - """ - _, notify_abort = notify_cancel - if not self._is_connected: - notify_abort() - return - if self._notify_cancels.setdefault(handle, notify_cancel) is not notify_cancel: - # A start_notify after the pop already owns this handle, so the - # popped entry is stale: nothing will retry its release. - _LOGGER.debug( - "%s: Notifications were re-enabled on handle %s while the " - "release was failing; discarding the stale entry", - self._description, - handle, - ) - notify_abort() + await self._async_write_cccd( + characteristic.handle, + cccd_descriptor, + CCCD_DISABLE_BYTES, + GATT_NOTIFY_TIMEOUT, + ) def _raise_if_not_connected(self) -> None: """Raise a BleakError if not connected.""" diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index 5b891e0..5f89d82 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -599,7 +599,7 @@ async def test_stop_notify_cccd_failure_survives_failing_release( esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, caplog: pytest.LogCaptureFixture, ) -> None: - """A failing release keeps the entry so the caller can retry it.""" + """A failing release is logged and does not mask the CCCD error.""" client, char = await _connected_client_with_char( client_data, esphome_bluetooth_gatt_services ) @@ -616,46 +616,12 @@ async def test_stop_notify_cccd_failure_survives_failing_release( ): await client.stop_notify(char) stop.assert_awaited_once() - assert char.handle in client._notify_cancels + assert char.handle not in client._notify_cancels # The CCCD failure is the actionable root cause the caller gets; the # secondary release failure is reported through the log. assert "Failed to release the proxy notify subscription" in caplog.text -@pytest.mark.asyncio -async def test_stop_notify_drops_entry_when_release_fails_after_disconnect( - client_data: ESPHomeClientData, - esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, -) -> None: - """A disconnect during the CCCD write must not resurrect the entry.""" - client, char = await _connected_client_with_char( - client_data, esphome_bluetooth_gatt_services - ) - stop = AsyncMock(side_effect=RuntimeError("release failed")) - abort = Mock() - client._notify_cancels[char.handle] = (stop, abort) - - async def _disconnect_and_raise(*args: Any, **kwargs: Any) -> None: - client._async_disconnected_cleanup() - raise BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)) - - with ( - patch.object( - client._client, - "bluetooth_gatt_write_descriptor", - side_effect=_disconnect_and_raise, - ), - pytest.raises(BleakError), - ): - await client.stop_notify(char) - stop.assert_awaited_once() - abort.assert_called_once() - assert char.handle not in client._notify_cancels - # The cleanup ran inside the write, so recording the handle afterwards - # would leave it dirty for the life of the next connection. - assert not client._cccd_dirty - - @pytest.mark.asyncio async def test_stop_notify_forgets_cccd_when_disconnected_mid_write( client_data: ESPHomeClientData, @@ -685,32 +651,6 @@ async def _disconnect_and_time_out(*args: Any, **kwargs: Any) -> None: assert not client._cccd_dirty -@pytest.mark.asyncio -async def test_stop_notify_release_failure_keeps_newer_subscription( - client_data: ESPHomeClientData, -) -> None: - """A restored entry never clobbers a subscription made after the pop.""" - client = _make_client(client_data) - client._is_connected = True - client._feature_flags &= ~BluetoothProxyFeature.REMOTE_CACHING.value - newer = (AsyncMock(), Mock()) - - async def _resubscribe() -> None: - client._notify_cancels[99] = newer - raise RuntimeError("release failed") - - char = Mock() - char.handle = 99 - abort = Mock() - client._notify_cancels[99] = (_resubscribe, abort) - with pytest.raises(RuntimeError): - await client.stop_notify(char) - assert client._notify_cancels[99] is newer - # The popped entry lost the handle, so it is aborted rather than left - # dangling with nothing to retry its release. - abort.assert_called_once() - - @pytest.mark.asyncio async def test_stop_notify_cccd_failure_survives_cancelled_release( client_data: ESPHomeClientData, @@ -732,14 +672,14 @@ async def test_stop_notify_cccd_failure_survives_cancelled_release( ): await client.stop_notify(char) stop.assert_awaited_once() - assert char.handle in client._notify_cancels + assert char.handle not in client._notify_cancels @pytest.mark.asyncio -async def test_stop_notify_keeps_entry_when_release_fails( +async def test_stop_notify_raises_when_release_fails( client_data: ESPHomeClientData, ) -> None: - """A failing release on the success path stays retryable.""" + """A failing release on the success path reaches the caller.""" client = _make_client(client_data) client._is_connected = True client._feature_flags &= ~BluetoothProxyFeature.REMOTE_CACHING.value @@ -750,7 +690,7 @@ async def test_stop_notify_keeps_entry_when_release_fails( with pytest.raises(RuntimeError): await client.stop_notify(char) stop.assert_awaited_once() - assert 99 in client._notify_cancels + assert 99 not in client._notify_cancels @pytest.mark.asyncio @@ -1073,6 +1013,67 @@ async def test_stop_notify_retries_cccd_after_a_failed_clear( mock_write_desc.assert_not_called() +@pytest.mark.asyncio +async def test_start_notify_cccd_failure_is_cleared_by_stop_notify( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A failed enable write may have landed, so stop_notify clears it.""" + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) + cccd = char.get_descriptor(CCCD_UUID) + assert cccd is not None + with ( + patch.object( + client._client, + "bluetooth_gatt_start_notify", + return_value=(AsyncMock(), Mock()), + ), + patch.object( + client._client, + "bluetooth_gatt_write_descriptor", + side_effect=BluetoothGATTAPIError(BluetoothGATTError(address=1, handle=2)), + ), + patch.object(client._client, "bluetooth_gatt_stop_notify"), + pytest.raises(BleakError), + ): + await client.start_notify(char, lambda data: None) + assert char.handle not in client._notify_cancels + assert char.handle in client._cccd_dirty + with patch.object( + client._client, "bluetooth_gatt_write_descriptor" + ) as mock_write_desc: + await client.stop_notify(char) + mock_write_desc.assert_awaited_once_with( + client._address_as_int, cccd.handle, CCCD_DISABLE_BYTES, GATT_NOTIFY_TIMEOUT + ) + assert char.handle not in client._cccd_dirty + + +@pytest.mark.asyncio +async def test_stop_notify_retry_racing_start_notify_stays_dirty( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A resubscribe during the retry keeps the handle dirty.""" + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) + client._cccd_dirty.add(char.handle) + + async def _resubscribe(*args: Any, **kwargs: Any) -> None: + client._notify_cancels[char.handle] = (AsyncMock(), Mock()) + + # The concurrent start_notify writes the same descriptor with no ordering + # guarantee, so the clear cannot claim the CCCD is off. + with patch.object( + client._client, "bluetooth_gatt_write_descriptor", side_effect=_resubscribe + ): + await client.stop_notify(char) + assert char.handle in client._cccd_dirty + + @pytest.mark.asyncio async def test_disconnect_forgets_the_outstanding_cccd_clear( client_data: ESPHomeClientData, From 0c064204635671acfd4de46833d354b61919cd2d Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Thu, 20 Aug 2026 04:21:21 +0000 Subject: [PATCH 16/18] refactor: document the start_notify cccd contract and trim the notify helpers --- src/bleak_esphome/backend/client.py | 73 ++++++++++++----------------- 1 file changed, 29 insertions(+), 44 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index e541a2b..9c8e7a9 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -169,9 +169,8 @@ def __init__( self._mtu: int | None = None self._cancel_connection_state: Callable[[], None] | None = None self._notify_cancels: dict[int, _NotifyCancel] = {} - # Handles whose CCCD write may have landed while the local - # bookkeeping was unwound, so a later stop_notify retries the clear - # instead of returning the missing-handle no-op. + # Handles whose CCCD write may have landed while the local bookkeeping + # was unwound, so a later stop_notify retries the clear. self._cccd_dirty: set[int] = set() self._device_info = client_data.device_info self._feature_flags = device_info.bluetooth_proxy_feature_flags_compat( @@ -885,6 +884,13 @@ async def start_notify( proxy (the subscribe, and the CCCD write that follows it on v3/REMOTE_CACHING connections). Defaults to 30.0. + Note: + ---- + On a ``REMOTE_CACHING`` proxy a failed CCCD write may still have + landed, leaving the peripheral notifying. Callers that keep the + connection alive should call ``stop_notify`` on the same + characteristic to clear it. + """ self._raise_if_not_connected() timeout = kwargs.get("timeout", GATT_NOTIFY_TIMEOUT) @@ -962,14 +968,11 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: self._raise_if_not_connected() handle = characteristic.handle # Do not raise KeyError if notifications are not enabled on this characteristic - # to be consistent with the behavior of the BlueZ backend. The entry is popped - # up front so a second concurrent stop_notify on the same handle is the same - # no-op it has always been instead of issuing a duplicate CCCD write and a - # duplicate release. + # to be consistent with the behavior of the BlueZ backend. Popping up front + # keeps a concurrent second stop_notify the no-op it has always been. if not (notify_cancel := self._notify_cancels.pop(handle, None)): - # The proxy-side subscription is already gone, but an earlier CCCD - # write may have failed. Retry it instead of returning a success - # while the peripheral is still notifying. + # The subscription is already gone, but an earlier CCCD write may + # have failed. Retry it instead of reporting success. if handle in self._cccd_dirty: await self._async_clear_cccd(characteristic) return @@ -982,10 +985,6 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: # Use BaseException to handle CancelledError as well as Exception. # Release the proxy-side subscription anyway, but let the CCCD # failure reach the caller: it is the actionable root cause. - # aioesphomeapi's stop_notify is an idempotent, synchronous body in - # an async wrapper, so it only fails once the API connection is - # gone -- there is nothing to retry and the disconnect cleanup owns - # the rest. try: await notify_stop() except Exception: @@ -1005,13 +1004,9 @@ def _get_cccd( """ Return the client config descriptor this host has to write itself. - ``None`` on firmware without ``REMOTE_CACHING``: the esp32 resolved - the descriptors and drives the CCCD itself, so neither - ``start_notify`` nor ``stop_notify`` touches it. On connection v3 the - esp32 skipped that resolution to save memory, so a characteristic - without a CCCD is an error: subscribing would never reach the - peripheral, and unsubscribing would leave it notifying for the life - of the connection. + ``None`` without ``REMOTE_CACHING``: the esp32 resolved the + descriptors and drives the CCCD itself. On connection v3 it skipped + that resolution to save memory, so a missing CCCD is an error. """ if not self._feature_flags & BluetoothProxyFeature.REMOTE_CACHING.value: return None @@ -1024,22 +1019,20 @@ def _get_cccd( async def _async_write_cccd( self, - handle: int, + char_handle: int, cccd_descriptor: BleakGATTDescriptor, value: bytes, timeout: float, ) -> None: """ - Write ``value`` to the client config descriptor of ``handle``. + Write ``value`` to the client config descriptor of ``char_handle``. A failed write may still have landed on the peripheral, so the characteristic handle is recorded in ``_cccd_dirty`` and a later ``stop_notify`` clears the descriptor instead of taking the - missing-handle no-op. Only while the link is up: the disconnect - cleanup may have run inside the await and a ``TimeoutAPIError`` or - ``CancelledError`` does not trigger a second one, so an unguarded add - would survive into the next connection -- which has nothing left to - retry anyway. + missing-handle no-op. Only while the link is up: a disconnect clears + the set, so an unguarded add would survive into the next connection, + which has nothing left to retry. """ _LOGGER.debug( "%s: Writing %s to CCD descriptor %s", @@ -1056,16 +1049,14 @@ async def _async_write_cccd( ) except BaseException: if self._is_connected: - self._cccd_dirty.add(handle) + self._cccd_dirty.add(char_handle) raise - if handle in self._notify_cancels: - # A start_notify owns the handle: either this write is its own - # enable, or one raced a clear and has a write in flight against - # the same descriptor with no ordering guarantee. Leave the handle - # dirty so the next stop_notify clears it again instead of - # declaring a descriptor it may have lost the race for clean. + if char_handle in self._notify_cancels: + # A start_notify owns the handle and may have a write in flight + # against the same descriptor with no ordering guarantee, so leave + # it dirty for the next stop_notify rather than declaring it clean. return - self._cccd_dirty.discard(handle) + self._cccd_dirty.discard(char_handle) async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> None: """ @@ -1073,14 +1064,8 @@ async def _async_clear_cccd(self, characteristic: BleakGATTCharacteristic) -> No Mirror of the connection v3 branch in ``start_notify``: the esp32 never resolved the descriptors, so it cannot undo the CCCD write - either. Without this the proxy stops forwarding the notifications - but the peripheral keeps sending them for the life of the - connection. - - ``_async_write_cccd`` tracks whether the write landed, so a - ``stop_notify`` that runs after the proxy-side subscription was - already released can tell a peripheral left notifying apart from a - handle that was never subscribed. + either. Without this the proxy stops forwarding the notifications but + the peripheral keeps sending them for the life of the connection. """ if (cccd_descriptor := self._get_cccd(characteristic)) is None: return From af25b5b26a23bc44baba56809d16fd2e2f0b9f59 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Thu, 20 Aug 2026 04:44:37 +0000 Subject: [PATCH 17/18] docs: correct the stop_notify retry contract and log the cccd characteristic handle --- src/bleak_esphome/backend/client.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 9c8e7a9..20e2834 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -955,7 +955,9 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: descriptor is cleared so the peripheral stops notifying. That is a round trip, so this method can block for up to the proxy GATT timeout and can raise ``BleakError`` where it previously always - returned; calling ``stop_notify`` again retries the write. + returned; calling ``stop_notify`` again retries a write that + failed. A characteristic with no client config descriptor raises + on every call and has nothing to retry. Args: ---- @@ -1035,10 +1037,11 @@ async def _async_write_cccd( which has nothing left to retry. """ _LOGGER.debug( - "%s: Writing %s to CCD descriptor %s", + "%s: Writing %s to CCD descriptor %s for characteristic %s", self._description, value, cccd_descriptor.handle, + char_handle, ) try: await self._client.bluetooth_gatt_write_descriptor( From 860d69a7f94a5df2d06f63044a903f1c52659831 Mon Sep 17 00:00:00 2001 From: Bluetooth Devices Bot Date: Thu, 20 Aug 2026 04:58:38 +0000 Subject: [PATCH 18/18] refactor: restore the notify release on failure and extract the release helper --- src/bleak_esphome/backend/client.py | 37 ++++++++++++++++++++++----- tests/backend/test_client_branches.py | 36 +++++++++++++++++++++++--- 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/src/bleak_esphome/backend/client.py b/src/bleak_esphome/backend/client.py index 20e2834..c73d199 100644 --- a/src/bleak_esphome/backend/client.py +++ b/src/bleak_esphome/backend/client.py @@ -955,9 +955,10 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: descriptor is cleared so the peripheral stops notifying. That is a round trip, so this method can block for up to the proxy GATT timeout and can raise ``BleakError`` where it previously always - returned; calling ``stop_notify`` again retries a write that - failed. A characteristic with no client config descriptor raises - on every call and has nothing to retry. + returned. Calling ``stop_notify`` again retries whatever failed: + the CCCD write, and the proxy-side release, which is put back + under its handle when it raises. A characteristic with no client + config descriptor raises on every call and has nothing to retry. Args: ---- @@ -978,7 +979,6 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: if handle in self._cccd_dirty: await self._async_clear_cccd(characteristic) return - notify_stop, _ = notify_cancel try: # Write the CCCD first so the peripheral is quiet before the # proxy-side subscription goes away. @@ -986,9 +986,10 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: except BaseException: # Use BaseException to handle CancelledError as well as Exception. # Release the proxy-side subscription anyway, but let the CCCD - # failure reach the caller: it is the actionable root cause. + # failure reach the caller: it is the actionable root cause. The + # release keeps its own retry affordance, so nothing is lost. try: - await notify_stop() + await self._async_release_notify(handle, notify_cancel) except Exception: _LOGGER.warning( "%s: Failed to release the proxy notify subscription for " @@ -998,7 +999,29 @@ async def stop_notify(self, characteristic: BleakGATTCharacteristic) -> None: exc_info=True, ) raise - await notify_stop() + await self._async_release_notify(handle, notify_cancel) + + async def _async_release_notify( + self, handle: int, notify_cancel: _NotifyCancel + ) -> None: + """ + Release the proxy-side notify subscription for ``handle``. + + The handle is popped from ``_notify_cancels`` before the CCCD write, + so a failed release would strand the subscription: nothing left for + ``_async_disconnected_cleanup`` to abort and nothing for a later + ``stop_notify`` to retry. Put the pair back so both paths reach it + again -- unless the link went down, since the cleanup already ran and + an unguarded restore would survive into the next connection, or a + ``start_notify`` took the handle over in the meantime. + """ + notify_stop, _ = notify_cancel + try: + await notify_stop() + except BaseException: + if self._is_connected: + self._notify_cancels.setdefault(handle, notify_cancel) + raise def _get_cccd( self, characteristic: BleakGATTCharacteristic diff --git a/tests/backend/test_client_branches.py b/tests/backend/test_client_branches.py index 5f89d82..36005da 100644 --- a/tests/backend/test_client_branches.py +++ b/tests/backend/test_client_branches.py @@ -16,7 +16,7 @@ import logging from collections.abc import Iterator from typing import Any -from unittest.mock import AsyncMock, Mock, patch +from unittest.mock import ANY, AsyncMock, Mock, patch import pytest from aioesphomeapi import ( @@ -616,12 +616,39 @@ async def test_stop_notify_cccd_failure_survives_failing_release( ): await client.stop_notify(char) stop.assert_awaited_once() - assert char.handle not in client._notify_cancels + # The failed release is put back so a retry and the disconnect cleanup + # can still reach it. + assert client._notify_cancels[char.handle] == (stop, ANY) # The CCCD failure is the actionable root cause the caller gets; the # secondary release failure is reported through the log. assert "Failed to release the proxy notify subscription" in caplog.text +@pytest.mark.asyncio +async def test_stop_notify_release_failure_after_disconnect_is_not_restored( + client_data: ESPHomeClientData, + esphome_bluetooth_gatt_services: ESPHomeBluetoothGATTServices, +) -> None: + """A release failing after the link dropped must not outlive it.""" + client, char = await _connected_client_with_char( + client_data, esphome_bluetooth_gatt_services + ) + + async def _disconnect_and_raise() -> None: + client._is_connected = False + raise RuntimeError("release failed") + + stop = AsyncMock(side_effect=_disconnect_and_raise) + client._notify_cancels[char.handle] = (stop, Mock()) + with ( + patch.object(client._client, "bluetooth_gatt_write_descriptor"), + pytest.raises(RuntimeError), + ): + await client.stop_notify(char) + stop.assert_awaited_once() + assert char.handle not in client._notify_cancels + + @pytest.mark.asyncio async def test_stop_notify_forgets_cccd_when_disconnected_mid_write( client_data: ESPHomeClientData, @@ -672,7 +699,8 @@ async def test_stop_notify_cccd_failure_survives_cancelled_release( ): await client.stop_notify(char) stop.assert_awaited_once() - assert char.handle not in client._notify_cancels + # A cancelled release is retryable too, so the pair goes back. + assert client._notify_cancels[char.handle] == (stop, ANY) @pytest.mark.asyncio @@ -690,7 +718,7 @@ async def test_stop_notify_raises_when_release_fails( with pytest.raises(RuntimeError): await client.stop_notify(char) stop.assert_awaited_once() - assert 99 not in client._notify_cancels + assert client._notify_cancels[99] == (stop, ANY) @pytest.mark.asyncio