Skip to content

fix(coordinator): prioritize event label intent and sync push provider lock state - #701

Open
firstof9 wants to merge 6 commits into
FutureTense:mainfrom
firstof9:fix/coordinator-lock-event-and-push-state
Open

fix(coordinator): prioritize event label intent and sync push provider lock state#701
firstof9 wants to merge 6 commits into
FutureTense:mainfrom
firstof9:fix/coordinator-lock-event-and-push-state

Conversation

@firstof9

@firstof9 firstof9 commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Summary of Changes

  • Prioritize explicit event label intent ("unlock" / "lock") in _handle_provider_lock_event over lock entity state mismatch.
  • Synchronize kmlock.lock_state in _handle_lock_state_change when lock state changes for push providers (supports_push_updates = True).
  • Add unit test coverage in tests/test_coordinator.py.

Ref #699

@codecov-commenter

codecov-commenter commented Aug 2, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.93%. Comparing base (cdb4922) to head (09f770d).
⚠️ Report is 221 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

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     
Flag Coverage Δ
python 93.83% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@firstof9
firstof9 requested review from raman325 and tykeal August 2, 2026 03:04
@secondof9

This comment was marked as resolved.

@tykeal tykeal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: when uses_provider_lock_events is true, assign kmlock.lock_state = UNLOCKED / LOCKED after the autolock branches, and zero _last_unlock_code_slot on unlock.
  • tests/test_coordinator.py — flip two existing assertions to the new synced state; add test_handle_provider_lock_event_prioritizes_event_label and test_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).

Comment thread custom_components/keymaster/coordinator.py
Comment thread custom_components/keymaster/coordinator.py
Comment thread tests/test_coordinator.py
Comment thread tests/test_coordinator.py
@firstof9

firstof9 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for highlighting the 3 blockers in the review summary:

  1. Precedence order in _handle_provider_lock_event: Updated _handle_provider_lock_event in commit 1f9db53 so explicit "unlock" / "lock" intent in event_label is evaluated before checking state_changed. Updated test_handle_provider_lock_event_prioritizes_event_label to set a stale kmlock.lock_state (UNLOCKED while entity state is LOCKED) to verify event label precedence when state_changed is true.

  2. Push provider scope: Added test coverage in commit 23abe6d for non-push providers (supports_push_updates = False) and provider = None via test_handle_lock_state_change_non_push_provider_does_not_sync_state to ensure kmlock.lock_state is only mutated for push-capable providers.

  3. Refactor: Pipeline Keymaster refreshes through dirty lock scopes #695 Interaction: Verified compatibility with notifications scheduling across provider updates.

All 167 coordinator tests pass cleanly.

@tykeal

tykeal commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

main has moved; this now conflicts only in custom_components/keymaster/coordinator.py inside _handle_lock_state_change, while tests/test_coordinator.py auto-merges cleanly. #695 removed async_schedule_global_notification() on main; resolve both hunks by keeping main's scoped self.async_schedule_keymaster_notifications([kmlock.keymaster_config_entry_id]) and also keeping the uses_provider_lock_events state-sync additions, because taking ours-only drops the push-provider fix and taking theirs-only calls a removed method/reintroduces global fan-out.

<<<<<<< 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

@firstof9
firstof9 force-pushed the fix/coordinator-lock-event-and-push-state branch from 385f55a to 4c6cbba Compare August 5, 2026 21:43
@firstof9
firstof9 force-pushed the fix/coordinator-lock-event-and-push-state branch from 4c6cbba to 09f770d Compare August 5, 2026 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants