fix(coordinator): prioritize event label intent and sync push provider lock state - #701
fix(coordinator): prioritize event label intent and sync push provider lock state#701firstof9 wants to merge 6 commits into
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #701 +/- ##
==========================================
+ Coverage 84.14% 93.93% +9.79%
==========================================
Files 10 42 +32
Lines 801 5408 +4607
Branches 0 30 +30
==========================================
+ Hits 674 5080 +4406
- Misses 127 328 +201
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
tykeal
left a comment
There was a problem hiding this comment.
Walkthrough
Adds a kmlock.lock_state synchronization to _handle_lock_state_change for push-capable providers, so the in-memory lock state no longer desyncs when a provider never emits a lock event (issue #699, bug 4). Also resets _last_unlock_code_slot to 0 on the unlock path, and updates/adds tests in tests/test_coordinator.py.
Changes
custom_components/keymaster/coordinator.py—_handle_lock_state_change: whenuses_provider_lock_eventsis true, assignkmlock.lock_state = UNLOCKED/LOCKEDafter the autolock branches, and zero_last_unlock_code_sloton unlock.tests/test_coordinator.py— flip two existing assertions to the new synced state; addtest_handle_provider_lock_event_prioritizes_event_labelandtest_handle_lock_state_change_syncs_push_provider_lock_state.
Review Comments
Blocking. The lock_state sync is a real fix for the desync, but as written it makes the push-provider event path order-dependent and silently disables its side effects when the entity state change is processed first. Details inline.
Cross-cutting
[BLOCKER] The stated _handle_provider_lock_event change is not in the diff.
The PR title and summary claim the event-label intent is now prioritized over entity-state mismatch, but _handle_provider_lock_event is untouched: state_changed is still evaluated first and still wins over an unambiguous "unlock" label (coordinator.py L861-871). Bug 3 of #699 is only indirectly mitigated — once lock_state stays in sync, state_changed is less likely to be spuriously true. That is a behavioural coincidence, not a precedence change, and it regresses the moment lock_state drifts again (e.g. a provider event is throttled, or the lock reports unavailable in between). Either implement the precedence change or correct the title/description so the changelog does not claim a fix that is not present.
[BLOCKER] Every provider is a push provider — the blast radius is 100% of installs.
supports_push_updates returns True unconditionally in zwave_js.py, zha.py, zigbee2mqtt.py and akuvox.py. There is no polled provider in-tree, so uses_provider_lock_events is true for every configured lock and both new branches execute for every user on every lock/unlock. This is not a Z2M-scoped change; it needs to be evaluated as a change to the Z-Wave JS and ZHA hot paths too.
[BLOCKER] Interaction with #695 (refactor/682-dirty-lock-refresh-pipeline).
(a) Missing scoped notification. Both new kmlock.lock_state mutations sit in an if uses_provider_lock_events: block that is not nested under the autolock elif, so they execute on paths where no notification is scheduled at all. lock_state is consumer-visible (e.g. switch.py L300 gates autolock-timer start on kmlock.lock_state == LockState.UNLOCKED). On main the global fan-out from unrelated events masks this. Under #695 notifications are scoped to dirty locks, so a mutation with no async_schedule_keymaster_notifications([kmlock.keymaster_config_entry_id]) is silent staleness. Once #695 lands, both new blocks need a scoped notification for kmlock.keymaster_config_entry_id.
(b) Merge conflict surface. Both of #701's hunks are anchored directly on self.async_schedule_global_notification() lines that #695 rewrites to self.async_schedule_keymaster_notifications([kmlock.keymaster_config_entry_id]) (#695's coordinator.py L908 and L923). Both hunks will conflict textually, and tests/test_coordinator.py conflicts as well (#701 edits the async_schedule_global_notification.assert_called_once() assertions in test_handle_lock_state_change_unlocked / _locked, which #695 also rewrites). Suggested landing order: merge #695 first (it is green and reviewed clean, and it is a pure refactor with no behavioural dependency on #701), then rebase #701 onto it and add the scoped notification calls. Rebasing #695 onto #701 would silently drop the notification requirement in (a).
|
Thanks for highlighting the 3 blockers in the review summary:
All 167 coordinator tests pass cleanly. |
|
main has moved; this now conflicts only in <<<<<<< upstream/main
self.async_schedule_keymaster_notifications([kmlock.keymaster_config_entry_id])
=======
self.async_schedule_global_notification()
if uses_provider_lock_events:
kmlock.lock_state = LockState.UNLOCKED
self._last_unlock_code_slot[kmlock.keymaster_config_entry_id] = 0
>>>>>>> upstream/pr/701
<<<<<<< upstream/main
self.async_schedule_keymaster_notifications([kmlock.keymaster_config_entry_id])
=======
self.async_schedule_global_notification()
if uses_provider_lock_events:
kmlock.lock_state = LockState.LOCKED
>>>>>>> upstream/pr/701 |
…ange and last_unlock_code_slot
385f55a to
4c6cbba
Compare
…e_provider_lock_event
4c6cbba to
09f770d
Compare
Summary of Changes
"unlock"/"lock") in_handle_provider_lock_eventover lock entity state mismatch.kmlock.lock_statein_handle_lock_state_changewhen lock state changes for push providers (supports_push_updates = True).tests/test_coordinator.py.Ref #699