fix: redesign autolock timer with separated lifecycle layers

.gitignore

@@ -23,3 +23,4 @@ package-lock.json
 .cursor/
 .aider*
 .continue/
+.serena/

custom_components/keymaster/autolock/__init__.py

@@ -0,0 +1,24 @@
+"""Autolock timer subsystem.
+
+Layered design:
+    store.py     — Persistence. Owns the asyncio.Lock; atomic store ops.
+    scheduler.py — Single async_call_later wrapper with awaitable cancel.
+    timer.py     — Orchestration. Explicit state machine. The public API.
+
+Public surface (importable from this package):
+    AutolockTimer, TimerEntry, TimerState — orchestration + types
+    TimerStore — persistence (one per coordinator)
+    TIMER_STORAGE_VERSION, TIMER_STORAGE_KEY — Store wiring
+"""
+
+from .store import TIMER_STORAGE_KEY, TIMER_STORAGE_VERSION, TimerEntry, TimerStore
+from .timer import AutolockTimer, TimerState
+
+__all__ = [
+    "TIMER_STORAGE_KEY",
+    "TIMER_STORAGE_VERSION",
+    "AutolockTimer",
+    "TimerEntry",
+    "TimerState",
+    "TimerStore",
+]

custom_components/keymaster/autolock/scheduler.py

@@ -0,0 +1,93 @@
+"""Single-shot scheduled callback wrapper with awaitable cancellation.
+
+Wraps `async_call_later` so callers can `await scheduled.cancel()` and
+be guaranteed that any in-flight execution of the callback has either
+completed or been cancelled before the await returns. This is what
+makes higher-layer cancel safe — without this, racing a cancellation
+against an already-running callback would let the callback's mutations
+land after the caller assumed it was stopped.
+"""
+
+from __future__ import annotations
+
+import asyncio
+from collections.abc import Awaitable, Callable
+from datetime import datetime as dt
+import logging
+
+from homeassistant.core import HomeAssistant
+from homeassistant.helpers.event import async_call_later
+
+_LOGGER: logging.Logger = logging.getLogger(__name__)
+
+
+class ScheduledFire:
+    """A single-shot scheduled async callback.
+
+    Instances are use-once: either the callback fires (and the instance
+    becomes `done`) or `cancel()` is called (and the instance becomes
+    `cancelled`). Either way, no further state transitions occur.
+    """
+
+    def __init__(
+        self,
+        hass: HomeAssistant,
+        delay: float,
+        action: Callable[[dt], Awaitable[None]],
+    ) -> None:
+        """Schedule `action` to run after `delay` seconds.
+
+        Negative `delay` is honored as "fire on the next event-loop tick".
+        """
+        self._hass = hass
+        self._action = action
+        self._task: asyncio.Task[None] | None = None
+        self._cancelled = False
+        self._done = False
+
+        async def _run(now: dt) -> None:
+            self._task = asyncio.current_task()
+            try:
+                await self._action(now)
+            finally:
+                self._task = None
+                self._done = True
+
+        self._unsub: Callable[[], None] | None = async_call_later(
+            hass=hass, delay=max(delay, 0), action=_run
+        )
+
+    async def cancel(self) -> None:
+        """Cancel the scheduled callback and wait for any in-flight run.
+
+        Idempotent. After this returns, the action is guaranteed to have
+        either completed (if it had already started) or been prevented
+        from starting (if `async_call_later` hadn't fired yet).
+        """
+        if self._cancelled:
+            return
+        self._cancelled = True
+        if self._unsub is not None:
+            self._unsub()
+            self._unsub = None
+        task = self._task
+        if task is not None and not task.done():
+            try:
+                await task
+            except asyncio.CancelledError:
+                # CancelledError inherits from BaseException; catch explicitly
+                # so it doesn't propagate and cancel our caller (cancel()'s
+                # contract is to not re-raise from in-flight failures).
+                _LOGGER.debug("[ScheduledFire] In-flight callback cancelled")
+            except Exception:
+                _LOGGER.exception("[ScheduledFire] In-flight callback raised during cancel")
+
+    @property
+    def cancelled(self) -> bool:
+        """Whether cancel() has been called."""
+        return self._cancelled
+
+    @property
+    def done(self) -> bool:
+        """Whether the callback finished executing."""
+        return self._done

custom_components/keymaster/autolock/store.py

@@ -0,0 +1,154 @@
+"""Persistent autolock timer store.
+
+One TimerStore instance per coordinator. Owns an asyncio.Lock so
+concurrent operations from different timers (which all write to the
+same disk store) can't interleave their load+modify+save sequences.
+
+The on-disk shape is `{timer_id: {"end_time": iso, "duration": int}}`.
+"""
+
+from __future__ import annotations
+
+import asyncio
+from collections.abc import Mapping
+from dataclasses import dataclass
+from datetime import datetime as dt
+import logging
+from typing import TypedDict
+
+from custom_components.keymaster.const import DOMAIN
+from homeassistant.core import HomeAssistant
+from homeassistant.helpers.storage import Store
+from homeassistant.util import dt as dt_util
+
+TIMER_STORAGE_VERSION = 1
+TIMER_STORAGE_KEY = f"{DOMAIN}.timers"
+
+_LOGGER: logging.Logger = logging.getLogger(__name__)
+
+
+class _TimerStoreEntryDict(TypedDict):
+    """Persisted shape for a single timer entry."""
+
+    end_time: str
+    duration: int
+
+
+@dataclass(frozen=True)
+class TimerEntry:
+    """A typed, validated timer entry.
+
+    `end_time` must be timezone-aware and `duration` non-negative;
+    enforced in `__post_init__` so all construction paths (parsed from
+    disk, built by callers) carry the same guarantees.
+    """
+
+    end_time: dt
+    duration: int
+
+    def __post_init__(self) -> None:
+        """Validate the entry on construction (see class docstring)."""
+        if self.end_time.tzinfo is None:
+            raise ValueError("TimerEntry.end_time must be timezone-aware")
+        if self.duration < 0:
+            raise ValueError(f"TimerEntry.duration must be non-negative, got {self.duration}")
+
+
+class TimerStore:
+    """Atomic persistence layer for autolock timers.
+
+    All public methods are async and acquire a shared lock so concurrent
+    operations from different AutolockTimer instances writing to the
+    same disk store can't lose updates.
+    """
+
+    def __init__(self, hass: HomeAssistant) -> None:
+        """Initialize with a fresh asyncio.Lock and the HA Store handle."""
+        self._store: Store[dict[str, _TimerStoreEntryDict]] = Store(
+            hass, TIMER_STORAGE_VERSION, TIMER_STORAGE_KEY
+        )
+        self._lock = asyncio.Lock()
+
+    async def read(self, timer_id: str) -> TimerEntry | None:
+        """Return the entry for `timer_id`, or None if absent or invalid.
+
+        Invalid (corrupt) entries are removed as a side effect so a
+        subsequent read returns None cleanly.
+        """
+        async with self._lock:
+            data = await self._store.async_load() or {}
+            raw = data.get(timer_id)
+            if raw is None:
+                return None
+            entry = self._parse(timer_id, raw)
+            if entry is None:
+                # Corrupt — remove inline so callers don't see it again
+                del data[timer_id]
+                await self._store.async_save(data)
+            return entry
+
+    async def write(self, timer_id: str, entry: TimerEntry) -> None:
+        """Persist `entry` under `timer_id`, replacing any prior value."""
+        async with self._lock:
+            data = await self._store.async_load() or {}
+            data[timer_id] = {
+                "end_time": entry.end_time.isoformat(),
+                "duration": entry.duration,
+            }
+            await self._store.async_save(data)
+
+    async def remove(self, timer_id: str) -> None:
+        """Remove the entry for `timer_id` if present. Idempotent."""
+        async with self._lock:
+            data = await self._store.async_load() or {}
+            if timer_id in data:
+                del data[timer_id]
+                await self._store.async_save(data)
+
+    @staticmethod
+    def _parse(timer_id: str, raw: object) -> TimerEntry | None:
+        """Parse a raw store entry into a TimerEntry, or None if invalid.
+
+        `raw` is `object` (not the TypedDict) because legacy or manually-
+        edited stores may contain anything; we defensively type-check
+        rather than trust the on-disk shape.
+        """
+        if not isinstance(raw, Mapping):
+            _LOGGER.warning(
+                "[TimerStore] %s: persisted entry is not a mapping (%s); treating as absent",
+                timer_id,
+                type(raw).__name__,
+            )
+            return None
+        try:
+            end_time = dt.fromisoformat(raw["end_time"])
+        except (KeyError, TypeError, ValueError):
+            _LOGGER.warning(
+                "[TimerStore] %s: invalid persisted end_time, treating as absent",
+                timer_id,
+            )
+            return None
+        if end_time.tzinfo is None:
+            # Legacy/manually-edited entries may be naive. Interpret them
+            # as already-UTC (we always write UTC) — `dt_util.as_utc`
+            # would assume local/default tz, which is wrong for our data.
+            end_time = end_time.replace(tzinfo=dt_util.UTC)
+        try:
+            duration = int(raw.get("duration", 0))
+        except (TypeError, ValueError):
+            _LOGGER.warning(
+                "[TimerStore] %s: invalid persisted duration, treating as absent",
+                timer_id,
+            )
+            return None
+        try:
+            return TimerEntry(end_time=end_time, duration=duration)
+        except ValueError as exc:
+            # TimerEntry.__post_init__ enforces non-negative duration and
+            # tz-aware end_time; surface those as recoverable, not crashes.
+            _LOGGER.warning(
+                "[TimerStore] %s: invalid persisted entry (%s); treating as absent",
+                timer_id,
+                exc,
+            )
+            return None