diff --git a/docs/bluetooth_vehicles.md b/docs/bluetooth_vehicles.md index 8e65dd1..b163edf 100644 --- a/docs/bluetooth_vehicles.md +++ b/docs/bluetooth_vehicles.md @@ -74,6 +74,24 @@ Tradeoff: because these reads generate link traffic, they keep an already-awake car awake and defer vehicle sleep. If you want the vehicle to sleep while idle, disable keepalive or disconnect when you have no work for it. +## Connection Retry Budget + +`connect()` and `connect_if_needed()` make at most two connection attempts by +default. This allows one retry for a waking vehicle or weak signal while +limiting a failed connection to roughly 40 seconds before raising +`BluetoothTransportError`. In particular, this lets a `VehicleRouter` move to +its cloud fallback promptly when the vehicle is discoverable but all of its BLE +connection slots are occupied. + +Pass `max_attempts` explicitly when an environment needs a larger retry budget: + +```python +await vehicle.connect(max_attempts=4) +``` + +The underlying connector's per-attempt timeout is fixed at about 20 seconds, so +increasing this value increases the worst-case connection delay accordingly. + ## Pair Vehicle You can pair a `VehicleBluetooth` instance using the `pair` method. Here's a basic example to pair a `VehicleBluetooth` instance: diff --git a/tesla_fleet_api/tesla/vehicle/bluetooth.py b/tesla_fleet_api/tesla/vehicle/bluetooth.py index 1b01601..5ec045d 100644 --- a/tesla_fleet_api/tesla/vehicle/bluetooth.py +++ b/tesla_fleet_api/tesla/vehicle/bluetooth.py @@ -13,7 +13,7 @@ from bleak.backends.characteristic import BleakGATTCharacteristic from bleak.backends.device import BLEDevice from bleak.exc import BleakCharacteristicNotFoundError, BleakError -from bleak_retry_connector import MAX_CONNECT_ATTEMPTS, establish_connection +from bleak_retry_connector import establish_connection from cryptography.hazmat.primitives.asymmetric import ec from google.protobuf.message import DecodeError @@ -115,6 +115,10 @@ # every 20s keeps it alive ~10x longer. See AGENTS.md for the measured evidence. DEFAULT_KEEPALIVE_INTERVAL = 20.0 +# The connector's per-attempt timeout is fixed and unexposed. Keep one retry for +# transient failures without delaying Router fallback for its full default. +DEFAULT_CONNECT_ATTEMPTS = 2 + if TYPE_CHECKING: # Resolved dynamically as ``bleak.BleakClient``/``bleak.BleakScanner`` at # call time so habluetooth's late-installed multi-adapter wrappers win @@ -614,7 +618,7 @@ def get_device(self) -> BLEDevice | None: """Return the currently assigned BLE device, if one has been discovered.""" return self.device - async def connect(self, max_attempts: int = MAX_CONNECT_ATTEMPTS) -> None: + async def connect(self, max_attempts: int = DEFAULT_CONNECT_ATTEMPTS) -> None: """Connect to the Tesla BLE device.""" if not self.device: raise ValueError(f"BLEDevice {self.ble_name} has not been found or set") @@ -700,7 +704,9 @@ def _on_ble_disconnected(self, client: BleakClient) -> None: return self._set_connected(False) - async def connect_if_needed(self, max_attempts: int = MAX_CONNECT_ATTEMPTS) -> None: + async def connect_if_needed( + self, max_attempts: int = DEFAULT_CONNECT_ATTEMPTS + ) -> None: """Connect to the Tesla BLE device if not already connected.""" async with self._connect_lock: if not self.client or not self.client.is_connected: diff --git a/tests/test_ble_connect_retry_budget.py b/tests/test_ble_connect_retry_budget.py new file mode 100644 index 0000000..6412a10 --- /dev/null +++ b/tests/test_ble_connect_retry_budget.py @@ -0,0 +1,154 @@ +"""Regression tests for the BLE connect retry budget. + +``bleak_retry_connector``'s own default (``MAX_CONNECT_ATTEMPTS`` = 4) pairs +with its fixed ~20s per-attempt connect timeout, so a contended connection +slot (every phone/watch slot held) burns ~81s of real GATT connect attempts +before ``connect()`` finally raises ``BluetoothTransportError`` and a +``Router`` can fail over to cloud - indistinguishable from a hang. These +tests lock in that ``connect()``/``connect_if_needed()`` now default to a +smaller attempt budget (``DEFAULT_CONNECT_ATTEMPTS``) instead of +``bleak_retry_connector``'s own default, while still letting a caller pass a +larger ``max_attempts`` explicitly for a scenario that genuinely needs it. +""" + +from __future__ import annotations + +from typing import Any +from unittest import IsolatedAsyncioTestCase +from unittest.mock import AsyncMock, MagicMock, patch + +from bleak.exc import BleakError +from cryptography.hazmat.primitives.asymmetric import ec + +import tesla_fleet_api.tesla.vehicle.bluetooth as vehicle_bluetooth +from tesla_fleet_api.exceptions import BluetoothTransportError +from tesla_fleet_api.router import VehicleRouter +from tesla_fleet_api.tesla.vehicle.bluetooth import ( + DEFAULT_CONNECT_ATTEMPTS, + VehicleBluetooth, +) + +VIN = "5YJXCAE43LF123456" + + +def _make_vehicle() -> VehicleBluetooth[Any]: + parent = MagicMock() + parent.private_key = ec.generate_private_key(ec.SECP256R1()) + vehicle = VehicleBluetooth(parent, VIN) + vehicle.device = MagicMock() + vehicle._start_keepalive = AsyncMock() # type: ignore[method-assign] + return vehicle + + +def _make_connected_client() -> MagicMock: + client = MagicMock() + client.start_notify = AsyncMock() + client.disconnect = AsyncMock() + client.is_connected = True + return client + + +class ConnectRetryBudgetTests(IsolatedAsyncioTestCase): + """The default attempt budget must be cut, not left at the vendored 4.""" + + def test_default_is_smaller_than_bleak_retry_connectors_own_default( + self, + ) -> None: + from bleak_retry_connector import MAX_CONNECT_ATTEMPTS + + self.assertLess(DEFAULT_CONNECT_ATTEMPTS, MAX_CONNECT_ATTEMPTS) + # Still allows one retry - a bare single attempt would give a + # genuinely transient failure (car waking, weak RF) no second try. + self.assertGreaterEqual(DEFAULT_CONNECT_ATTEMPTS, 2) + + async def test_connect_passes_reduced_default_to_establish_connection( + self, + ) -> None: + vehicle = _make_vehicle() + establish = AsyncMock(return_value=_make_connected_client()) + + with patch.object(vehicle_bluetooth, "establish_connection", establish): + await vehicle.connect() + + self.assertEqual( + establish.call_args.kwargs["max_attempts"], DEFAULT_CONNECT_ATTEMPTS + ) + + async def test_connect_if_needed_passes_reduced_default(self) -> None: + vehicle = _make_vehicle() + establish = AsyncMock(return_value=_make_connected_client()) + + with patch.object(vehicle_bluetooth, "establish_connection", establish): + await vehicle.connect_if_needed() + + self.assertEqual( + establish.call_args.kwargs["max_attempts"], DEFAULT_CONNECT_ATTEMPTS + ) + + async def test_caller_can_still_override_for_a_larger_budget(self) -> None: + """A caller doing its own long-poll retry can still ask for more.""" + vehicle = _make_vehicle() + establish = AsyncMock(return_value=_make_connected_client()) + + with patch.object(vehicle_bluetooth, "establish_connection", establish): + await vehicle.connect(max_attempts=5) + + self.assertEqual(establish.call_args.kwargs["max_attempts"], 5) + + async def test_contended_slot_failure_surfaces_after_the_reduced_budget( + self, + ) -> None: + """A slot-exhausted vehicle (every attempt in the budget times out) + must still raise ``BluetoothTransportError`` - only the budget + handed to ``establish_connection`` shrinks, not the exception + contract a ``Router`` fails over on.""" + vehicle = _make_vehicle() + # bleak_retry_connector exhausts the whole budget internally and + # raises a single BleakError once max_attempts is used up. + establish = AsyncMock( + side_effect=BleakError("device not found: out of connection slots") + ) + + with patch.object(vehicle_bluetooth, "establish_connection", establish): + with self.assertRaises(BluetoothTransportError): + await vehicle.connect() + + establish.assert_awaited_once() + self.assertEqual( + establish.call_args.kwargs["max_attempts"], DEFAULT_CONNECT_ATTEMPTS + ) + + +class _FakeCloudFallback: + """A cloud secondary tracking whether the router fell over to it.""" + + def __init__(self) -> None: + self.vin = VIN + self.wake_up_calls = 0 + + async def wake_up(self) -> dict[str, Any]: + self.wake_up_calls += 1 + return {"response": {"result": True, "reason": ""}} + + +class ContendedSlotFailsOverFastTests(IsolatedAsyncioTestCase): + """A contended-slot connect failure must still fail over to cloud - the + reduced budget only changes how long that takes, not whether it works.""" + + async def test_router_fails_over_after_reduced_connect_budget(self) -> None: + primary = _make_vehicle() + establish = AsyncMock( + side_effect=BleakError("device not found: out of connection slots") + ) + fallback = _FakeCloudFallback() + router = VehicleRouter(primary, fallback) + + with patch.object(vehicle_bluetooth, "establish_connection", establish): + result = await router.wake_up() + + self.assertEqual(result, {"response": {"result": True, "reason": ""}}) + self.assertEqual(fallback.wake_up_calls, 1) + # Only one establish_connection call for the whole failed primary + # attempt - the reduced max_attempts is what bounds its internal + # retry loop, not repeated calls from our code. + establish.assert_awaited_once()