feat(ble): process-global per-MAC connection lock with contention WARNING#67
Open
davelee98 wants to merge 1 commit into
Open
feat(ble): process-global per-MAC connection lock with contention WARNING#67davelee98 wants to merge 1 commit into
davelee98 wants to merge 1 commit into
Conversation
…NING The tag exposes a single BLE link, but serialization lived on runtime_data.ble_lock — per config entry and created at setup — so the two config-read paths connected unguarded: the config-flow probe (no entry exists yet) and the entry-setup interrogation (runs before runtime_data is constructed). Overlapping connects on a local BlueZ dongle are a real trigger for the getConnectionCount()==0/subscribers==1 state in the dongle-only config-read investigation (2026-07-16). Add ble_lock.py: a WeakValueDictionary registry of asyncio.Locks keyed by format_mac(address), plus a ble_connection(address, purpose) context manager that WARNs — naming both the waiting purpose and the current holder — when a second connection is attempted while the lock is held. The weak map is load-bearing: asyncio.Lock binds to the loop that first acquires it, and per-test event loops would otherwise inherit dead-loop locks through the shared ADDRESS constant. All five connect sites now acquire it: config-flow probe and setup interrogation (newly guarded, inside their existing deadlines), queued delivery drain (outside its timeout — polite unbounded wait preserved), service calls, and OTA (outer acquisition only; the nested app-mode and AppLoader connects stay unwrapped — the lock is not reentrant). RuntimeData.ble_lock is now required and set to the registry lock, so the reboot/unload drains work unchanged and the lock survives reloads. Tests: new test_ble_lock.py (case-variant sharing, contention WARNING, holder cleanup, weak collection); delivery/services suites assert the registry lock is held during connects and that a contended drain waits, warns once, and completes after release. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
🔗 Companion to py-opendisplay PR #138 (BLE watcher-leak fix)
This lock is one half of the dongle-only config-read fix. The other half is the client-side notification-watcher-leak fix in OpenDisplay/py-opendisplay#138 (
fix(transport): close all BLE notification-watcher leak paths). The two were validated together on the debug branch; the dongle failure only fully clears with both in place.This PR intentionally does not touch
manifest.json— it does not bump the py-opendisplay pin, so it stays mergeable on its own against the current pinned release. Recommended follow-up: once #138 is merged and released, bump the py-opendisplay pin in a separate PR to pull in the watcher fix and complete the dongle fix end-to-end.What
Adds a process-global, per-MAC BLE connection lock so overlapping host-side connects to one tag are serialized. An OpenDisplay tag exposes a single BLE link and py-opendisplay holds no per-address lock, so two operations connecting to the same MAC at once race and wedge the link (worst on a local BlueZ dongle).
The prior serialization lived on
OpenDisplayRuntimeData.ble_lock— per config entry, created at setup — which left the two config-read paths unguarded: the config-flow probe (no entry exists yet) and entry-setup interrogation (runs beforeruntime_datais built).Why
Traced from a dongle-only config-read failure where the ESP32 showed
getConnectionCount()==0alongsidesubscribers==1— the fingerprint of connection churn / overlapping links on a single-connection peripheral. Empirically, this lock resolved the dongle config-read failure. The ESPHome-proxy path was unaffected because it serializes connects; the local BlueZ dongle did not.How
ble_lock.py: aWeakValueDictionary[str, asyncio.Lock]registry keyed byformat_mac(address), plus anasync_get_ble_lock()getter and able_connection(address, purpose)async context manager. It emits a WARNING before awaiting whenever the link is already held, naming both the waiting operation and the current holder — so overlapping connects are diagnosable in the log.WeakValueDictionaryis load-bearing (documented in the module):asyncio.Lockbinds to the loop that first acquires it, so a plain dict would hand a dead-loop lock to the next pytest-asyncio test (fresh loop per test, sharedADDRESS).OpenDisplayRuntimeData.ble_lockis now a required field set to the shared registry lock, so the reboot/unload drains work unchanged and the lock survives entry reloads.Process-global + per-MAC is deliberate: different tags stay fully concurrent; only same-MAC connects serialize.
Testing
tests/test_ble_lock.py: case-variant address sharing, distinct-MAC isolation, contended serialize + single WARNING (naming waiter + holder), uncontended silence, holder cleanup, weak collection.tests/test_delivery.py/tests/test_services.py: assert the registry lock is held during a connect and that a contended drain/send waits, warns once, and completes after release.ruff check .passes.pytest tests/suite imports Home Assistant and must be run in the devcontainer — please confirm green in CI.