fix(coordinator): coalesce parent-child sync notifications to dirty locks - #707
fix(coordinator): coalesce parent-child sync notifications to dirty locks#707tykeal wants to merge 4 commits into
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #707 +/- ##
===========================================
+ Coverage 84.14% 94.25% +10.10%
===========================================
Files 10 42 +32
Lines 801 5480 +4679
Branches 0 30 +30
===========================================
+ Hits 674 5165 +4491
- Misses 127 315 +188
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:
|
e0381cf to
16d633a
Compare
…ocks Add a _parent_sync_transaction async context manager that suppresses per-slot async_schedule_keymaster_notifications fan-out during the _sync_child_locks loop. Instead, entry IDs are accumulated in a transaction-scoped set and flushed as a single notification on exit. The transaction is: - Re-entrant via a depth counter (nested uses share the accumulator) - Exception-safe via try/finally (depth always decrements) - Preserves all_entry_ids=True semantics through the deferral This eliminates O(slots × children) notification scheduling during parent→child synchronization, replacing it with exactly one coalesced notification per sync cycle. Closes FutureTense#683 Assisted-by: GitHub Copilot CLI 1.0.75 (Claude Opus 5, model claude-opus-5) Signed-off-by: Andrew Grimberg <tykeal@bardicgrove.org> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the global _quick_refresh bool and single cancel handles with per-entry equivalents: - _quick_refresh_entry_ids: set[str] tracks which entries need refresh - _cancel_quick_refresh: dict[str, Callable] holds per-entry timer handles - _cancel_debounced_refresh: dict[str, Callable] holds per-entry debounce handles Each set_pin_on_lock / clear_pin_from_lock / _sync_pin / _update_child_code_slots now records only the affected entry ID. Timers fire per-entry through async_refresh_lock(entry_id) which serialises via _debounced_refresh.async_lock(), preventing independent simultaneous polling. Shutdown and _delete_lock cancel all/per-entry handles respectively, preventing dangling timers. Closes FutureTense#684 Assisted-by: GitHub Copilot CLI 1.0.75 (Claude Opus 5, model claude-opus-5) Signed-off-by: Andrew Grimberg <tykeal@bardicgrove.org> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… test name
- Replace truthiness-only assertions on _quick_refresh_entry_ids with
exact set equality (e.g. == {"child_id"}) to catch wrong-entry bugs
- Rename test_scoped_refresh_keeps_shared_debounce_for_other_dirty_locks
to test_scoped_refresh_preserves_other_entry_debounce_timer to reflect
the per-entry design
Assisted-by: GitHub Copilot CLI 1.0.75 (Claude Opus 5, model claude-opus-5)
Signed-off-by: Andrew Grimberg <tykeal@bardicgrove.org>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add tests for three previously-uncovered lines: - _trigger_debounced_refresh_for_entry: "_global" sentinel and lock-removed-before-timer-fires both fall back to full refresh - _clear_pending_quick_refresh(None): cancels ALL per-entry handles - _schedule_quick_refresh_if_needed: dedup guard skips entries that already have a pending timer (identity assertion on stored handle) Assisted-by: GitHub Copilot CLI 1.0.75 (Claude Opus 5, model claude-opus-5) Signed-off-by: Andrew Grimberg <tykeal@bardicgrove.org> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
16d633a to
17e1ba6
Compare
secondof9
left a comment
There was a problem hiding this comment.
📋 Review Summary
Tip
Review Status: 🟢 APPROVED
Change Type: 🛠️ Refactor / 🐛 Bug Fix
Review Effort: 🟢 Low
Core Impact: Clean coalescing of per-entry refresh timers and parent-child sync notifications. Defensive design with zero runtime hazards detected.
🚦 CI & Pipeline Health Summary
| Check / Workflow Name | Status | Impact on Review |
|---|---|---|
| coverage | ✅ PASSED | 94.25% — no regressions |
| Pytest (3.14) | ✅ PASSED | All 1092 tests passed, 1 deselected |
| Autolabel PR | ⏩ SKIPPED | Non-blocking |
| Prek | ✅ PASSED | Static analysis clean |
| Hassfest Validation | ✅ PASSED | Integration schema valid |
| HACS Validation | ✅ PASSED | Custom component compliant |
| Autolabel PR (2nd) | ✅ PASSED | Non-blocking |
Note
CI Pipeline Clear: All GitHub Actions workflows completed successfully.
🔍 Architectural Walkthrough
custom_components/keymaster/coordinator.py
-
_parent_sync_transactioncontext manager (L396-417)- Exception-safe re-entrant depth counter
- Accumulates dirty entry IDs during parent-child sync loop
- Flushes notification exactly once on outermost exit
- Preserves
all_entry_ids=Truesemantics through transaction - ✅ No silent resource leaks —
finallyblock handles depth decrement
-
Per-entry refresh state (L224-228, 235-238)
- Replaces global
_quick_refresh: boolwith dict[str, Callable] for quick-refresh and debounced-refresh - Prevents independent simultaneous polling of locks
- Shutdown cleanly cancels all per-entry handles
- ✅ Correct asyncio task lifecycle — no dangling timers
- Replaces global
-
_cancel_entry_refresh_timerscallback (L2629-2640)- Single-point cleanup for quick-refresh + debounced-refresh + pending sets
- Called from
_delete_lockto prevent dangling timers - ✅ Proper cleanup boundary
-
_async_refresh_lock_dataandasync_refresh_all_locks- Clear per-entry timers before/after refresh operations
_async_update_datacalls_clear_pending_quick_refresh(None)for full refresh- ✅ No orphaned async_call_later callbacks
🔗 Related / Outside-Diff Context
-
tests/test_coordinator.py: Per-entry refresh assertions upgraded from truthiness (assert coordinator._quick_refresh_entry_ids) to exact set equality (assert coordinator._quick_refresh_entry_ids == {"child_id"}). Tighter test coverage, catches wrong-entry-ID bugs the old assertions would miss. -
tests/test_coordinator_sync.py: NewTestParentSyncTransactionclass with 6 tests covering re-entrancy, exception safety, override-parent guard, and all_entry_ids preservation. Comprehensive coverage of the new transaction mechanism. -
tests/test_debounce.py: NewTestPerEntryQuickRefreshandTestPerEntryRefreshEdgeCasesclasses. Tests for dedup guards, reschedule prevention, and global-fallback paths. ✅ Mutation testing verified all new tests.
Review: APPROVED — fresh review of FutureTense/keymaster PR #707 (fix/coalesce-parent-child-sync-notifications). All CI green, no unresolved review threads, well-structured code with proper async boundaries and cleanup.
Summary
Delivers both #683 and #684 as a single releasable unit. Issue #683's description explicitly states "Do not ship this without task-10's quick/debounce scoping" (task-10 being #684), so both are bundled here.
What was already done by #695
PR #695 had already delivered 3 of #683's 5 acceptance criteria:
override_parentchildren not mutated/notified (continueguard in_update_child_code_slots; guards inset_pin_on_lock/clear_pin_from_lock)_defer_refresh_listener_updates,_active_refresh_count,_pending_notify_entry_ids, flush on refresh boundary)This PR implements only the genuinely outstanding parts. Reviewers should not expect a larger diff — the foundation was already in place.
Commits
1.
f7b9c18— fix(coordinator): coalesce parent-child sync notifications to dirty locksAdds
_parent_sync_transaction, an@asynccontextmanagerthat suppresses per-slotasync_schedule_keymaster_notificationsfan-out during the_sync_child_lockschild loop. Entry IDs are accumulated in a transaction-scopedset[str]and flushed as a single notification on exit. The context manager is re-entrant (depth counter), exception-safe (finallyblock), and preservesall_entry_ids=Truesemantics through the deferral.Design decision (repo owner): Intermediate
Synced.ADDING/Synced.DELETINGstates andsync_op_started_atare still written to the model exactly as before. Only the notification fan-out is suppressed. This deliberately preserves stuck-state recovery (TestSyncPinStuckStateRecovery) and provider-retry logic, which depend on those fields being set before the provider I/O call.2.
b47dc6a— fix(coordinator): scope quick and debounced refresh to per-entry timersReplaces the global
_quick_refresh: booland single_cancel_quick_refresh/_cancel_debounced_refreshhandles with per-entry equivalents:_quick_refresh_entry_ids: set[str]— tracks which entries need a follow-up refresh_cancel_quick_refresh: dict[str, Callable]— per-entry quick-refresh timer handles_cancel_debounced_refresh: dict[str, Callable]— per-entry debounce timer handlesEach
set_pin_on_lock/clear_pin_from_lock/_sync_pin/_update_child_code_slotsnow records only the affected entry ID. Per-entry timers fire throughasync_refresh_lock(entry_id), which serialises viaself._debounced_refresh.async_lock()— the same asyncio lock from the parentDataUpdateCoordinator. This guarantees no independent simultaneous polling is reintroduced.Shutdown and
_delete_lockcancel all/per-entry handles respectively, preventing danglingasync_call_latercallbacks.3.
e534907— test(coordinator): tighten per-entry refresh assertions and fix stale test nameReplaces truthiness-only assertions on
_quick_refresh_entry_ids(e.g.assert coordinator._quick_refresh_entry_ids) with exact set equality (e.g.== {"child_id"}). This catches wrong-entry-ID bugs that the old assertions would miss. Also renamestest_scoped_refresh_keeps_shared_debounce_for_other_dirty_locks→test_scoped_refresh_preserves_other_entry_debounce_timerto reflect the per-entry design.Validation
pytest tests/: 1092 passed, 1 deselectedruff check custom_components/ tests/: All checks passedruff format --check custom_components/ tests/: All formattedmypy custom_components/keymaster/: No issues foundMutation testing
All new tests were mutation-verified. Key mutations validated:
async_schedule_keymaster_notificationsfinallydepth decrement_update_child_code_slotscancel()calls during shutdownCloses #683
Closes #684