fix: prevent AttributeError race in autolock timer persistence#601
fix: prevent AttributeError race in autolock timer persistence#601raman325 wants to merge 3 commits intoFutureTense:mainfrom
Conversation
cancel() can null _end_time while _persist_to_store awaits async_load, causing an AttributeError on isoformat(). Snapshot values into locals before the first await so the persist uses captured state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #601 +/- ##
=======================================
Coverage ? 90.19%
=======================================
Files ? 28
Lines ? 3529
Branches ? 0
=======================================
Hits ? 3183
Misses ? 346
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a race in KeymasterTimer._persist_to_store() where cancel() can clear _end_time during an await, leading to AttributeError when persisting timer state (reported in #594).
Changes:
- Snapshot
_end_timeand_durationinto locals before the firstawaitin_persist_to_store(). - Add a regression test that simulates cancellation occurring during persistence.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
custom_components/keymaster/helpers.py |
Snapshots timer state before awaiting store I/O to avoid NoneType.isoformat() crashes. |
tests/test_helpers.py |
Adds an async test intended to reproduce the concurrent-cancel persistence crash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
After async_load yields, verify _end_time still matches the snapshot before saving. If cancel() ran during the await, bail out instead of re-adding the entry. Updates test to assert cancel wins the race. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replaces the snapshot+re-check approach with an asyncio.Lock shared between _persist_to_store and _remove_from_store, fully preventing interleaved store operations that could resurrect canceled timers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def test_keymaster_timer_persist_no_crash_when_end_time_cleared(hass, mock_store): | ||
| """Test _persist_to_store doesn't crash if _end_time is cleared before lock acquired.""" | ||
| timer = KeymasterTimer() | ||
| kmlock = KeymasterLock( | ||
| lock_name="test_lock", | ||
| lock_entity_id="lock.test_lock", | ||
| keymaster_config_entry_id="test_entry", | ||
| ) | ||
| kmlock.autolock_min_day = 5 | ||
|
|
||
| async def mock_action(*args): | ||
| pass | ||
|
|
||
| await timer.setup(hass, kmlock, mock_action, timer_id="test_timer", store=mock_store) | ||
|
|
||
| # Set timer state then clear it (simulates cancel() winning the pre-guard check) | ||
| timer._end_time = dt_util.utcnow() + timedelta(minutes=5) | ||
| timer._duration = 300 | ||
| # Acquire the lock first (simulates cancel holding it), then clear end_time | ||
| async with timer._store_lock: | ||
| timer._end_time = None | ||
| timer._duration = None | ||
|
|
||
| # Now persist runs — should bail out at the pre-guard, not crash | ||
| await timer._persist_to_store() | ||
| mock_store.async_save.assert_not_called() |
There was a problem hiding this comment.
The new test doesn’t actually reproduce the reported race/AttributeError: it clears _end_time before calling _persist_to_store(), so _persist_to_store exits via the initial guard (if ... not self._end_time) even on the pre-fix implementation. To validate the fix, coordinate two concurrent tasks so _persist_to_store() passes its initial guard, then yields on await self._store.async_load(), and only then cancel() clears _end_time before isoformat() would run (e.g., make mock_store.async_load await an asyncio.Event that cancel() sets).
| async def test_keymaster_timer_store_lock_serializes_persist_and_remove(hass, mock_store): | ||
| """Test that _store_lock prevents persist and remove from interleaving.""" | ||
| timer = KeymasterTimer() | ||
| kmlock = KeymasterLock( | ||
| lock_name="test_lock", | ||
| lock_entity_id="lock.test_lock", | ||
| keymaster_config_entry_id="test_entry", | ||
| ) | ||
| kmlock.autolock_min_day = 5 | ||
|
|
||
| async def mock_action(*args): | ||
| pass | ||
|
|
||
| await timer.setup(hass, kmlock, mock_action, timer_id="test_timer", store=mock_store) | ||
|
|
||
| with patch("custom_components.keymaster.helpers.sun.is_up", return_value=True): | ||
| await timer.start() | ||
|
|
||
| # Verify persist saved the timer | ||
| mock_store.async_save.assert_called() | ||
| saved_data = mock_store.async_save.call_args[0][0] | ||
| assert "test_timer" in saved_data | ||
|
|
||
| # Now cancel — remove should clean up the entry | ||
| mock_store.async_load = AsyncMock(return_value=dict(saved_data)) | ||
| mock_store.async_save.reset_mock() | ||
| await timer.cancel() | ||
|
|
||
| mock_store.async_save.assert_called() | ||
| final_data = mock_store.async_save.call_args[0][0] | ||
| assert "test_timer" not in final_data | ||
|
|
There was a problem hiding this comment.
This test’s name/docstring claims to verify _store_lock serialization, but the code only exercises the normal sequential start() then cancel() path (no concurrency), so it would pass even if _store_lock were removed. If the intent is to verify serialization, run _persist_to_store() and cancel() (or _remove_from_store()) concurrently and assert the ordering/absence of interleaving (for example, block async_save until the other coroutine attempts to run, then confirm it is forced to wait).
Summary
Fixes a race condition where
cancel()nulls_end_timewhile_persist_to_storeis mid-await, causingAttributeError: 'NoneType' object has no attribute 'isoformat'(reported in #594's latest comment).Proposed change
Adds an
asyncio.Lockshared between_persist_to_store()and_remove_from_store()to serialize all store operations on the timer. This preventscancel()from interleaving with an in-flight persist, which could either crash (the reported bug) or resurrect a canceled timer entry in storage.Type of change
Additional information
🤖 Generated with Claude Code