From 1a85f7a56d344c3bf13f616d1000abbafe3bd99b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 15 Jun 2026 08:33:53 -0500 Subject: [PATCH 1/6] Add a RAM-canonical PreferencesStore for user preferences --- .../controllers/config/__init__.py | 12 - .../controllers/config/_preferences_store.py | 213 ++++++++++++++ .../controllers/config/controller.py | 25 +- .../controllers/config/preferences.py | 55 +--- .../controllers/onboarding.py | 28 +- tests/controllers/config/__init__.py | 1 + .../config/test_preferences_store.py | 264 ++++++++++++++++++ tests/test_config_controller.py | 128 +++------ tests/test_onboarding_controller.py | 64 ++--- 9 files changed, 563 insertions(+), 227 deletions(-) create mode 100644 esphome_device_builder/controllers/config/_preferences_store.py create mode 100644 tests/controllers/config/__init__.py create mode 100644 tests/controllers/config/test_preferences_store.py diff --git a/esphome_device_builder/controllers/config/__init__.py b/esphome_device_builder/controllers/config/__init__.py index e61457dfa..444dece3c 100644 --- a/esphome_device_builder/controllers/config/__init__.py +++ b/esphome_device_builder/controllers/config/__init__.py @@ -56,13 +56,6 @@ rename_device_metadata, set_device_metadata, ) -from .preferences import ( - _prefs_from_data, - load_preferences, - mutate_preferences, - save_preferences, - update_preferences, -) from .remote_build_settings import ( _settings_from_raw, has_remote_build_settings_persisted, @@ -99,7 +92,6 @@ "_make_descriptor_tempfile", "_parse_chip_family_line", "_parse_project_name", - "_prefs_from_data", "_read_app_descriptor_board_id", "_read_descriptor_file", "_run_esptool", @@ -114,17 +106,13 @@ "has_remote_build_settings_persisted", "labels_transaction", "load_labels", - "load_preferences", "load_remote_build_settings", "metadata_transaction", - "mutate_preferences", "remote_build_settings_transaction", "remove_device_metadata", "rename_device_metadata", "save_labels", - "save_preferences", "save_remote_build_settings", "set_device_labels", "set_device_metadata", - "update_preferences", ] diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py new file mode 100644 index 000000000..82d0aa40b --- /dev/null +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -0,0 +1,213 @@ +"""RAM-canonical user preferences with a debounced disk write.""" + +from __future__ import annotations + +import asyncio +import logging +from collections.abc import Callable +from pathlib import Path +from typing import Any + +from ...helpers.json import JSONDecodeError, dumps_indent, loads +from ...helpers.storage import ShutdownRegister, Store +from ...models import UserPreferences +from .metadata import _load_metadata, metadata_transaction +from .preferences import _PREFS_KEY + +_LOGGER = logging.getLogger(__name__) + +_STORE_FILENAME = ".device-builder-preferences.json" +_SHARED_SIDECAR_FILENAME = ".device-builder.json" + +_DEFAULT_SAVE_DELAY = 1.0 + +# Decode failures that should be treated as "corrupt / incompatible": a bad-JSON +# read, a non-object payload, or a shape ``from_dict`` rejects. +_DECODE_ERRORS = (JSONDecodeError, ValueError, TypeError, LookupError) + + +def _encode(prefs: UserPreferences) -> bytes: + return dumps_indent(prefs.to_dict()) + + +def _decode(raw: bytes) -> UserPreferences: + """Decode stored preferences; raise on a corrupt or incompatible payload. + + Corruption propagates (rather than defaulting) so the caller can preserve + the file for recovery instead of silently overwriting it. + """ + obj = loads(raw) + if not isinstance(obj, dict): + raise TypeError("preferences payload is not a JSON object") + return UserPreferences.from_dict(obj) + + +class PreferencesStore: + """RAM-canonical user preferences; writes go through a debounced ``Store``.""" + + def __init__(self, config_dir: Path, shutdown_register: ShutdownRegister) -> None: + self._config_dir = config_dir + self._state = UserPreferences() + # Set when an undecodable file couldn't be renamed aside; suppresses all + # writes so a later save can't overwrite the still-recoverable corrupt file. + self._persist_disabled = False + self._store: Store[UserPreferences] = Store( + config_dir / _STORE_FILENAME, + encoder=_encode, + decoder=_decode, + shutdown_register=shutdown_register, + name="preferences", + ) + + async def async_load(self) -> None: + """Seed RAM from disk; migrate the sidecar's ``_preferences`` on first run. + + Undecodable data is preserved, never destroyed: a corrupt dedicated file + is renamed aside (then the legacy sidecar is still tried, so a recoverable + blob isn't lost), and an undecodable legacy blob is left in place. Both + fall back to defaults. + """ + loop = asyncio.get_running_loop() + try: + loaded = await self._store.async_load() + except _DECODE_ERRORS: + _LOGGER.exception( + "preferences store: %s is undecodable; preserving it and using defaults", + _STORE_FILENAME, + ) + await loop.run_in_executor(None, self._preserve_corrupt_file) + await self._migrate_from_sidecar(loop) + return + if loaded is not None: + self._state = loaded + return + await self._migrate_from_sidecar(loop) + + async def _migrate_from_sidecar(self, loop: asyncio.AbstractEventLoop) -> None: + """Adopt the legacy ``_preferences`` blob, persist it, then strip the key. + + The strip is gated on a confirmed write so an unconfirmed flush can't lose + the prefs on restart (see :meth:`_confirm_and_strip_shared_sync`). + """ + migrated = await loop.run_in_executor(None, self._migrate_read_shared_sync) + if migrated is None: + return + self._state = migrated + if self._persist_disabled: + return + self._store.async_delay_save(self._snapshot, delay=0.0) + await self._store.async_save_now() + stripped = await loop.run_in_executor(None, self._confirm_and_strip_shared_sync) + if not stripped: + _LOGGER.warning( + "preferences store: %s write unconfirmed; keeping %s in %s to retry", + _STORE_FILENAME, + _PREFS_KEY, + _SHARED_SIDECAR_FILENAME, + ) + return + _LOGGER.info( + "Migrated preferences from %s to %s", _SHARED_SIDECAR_FILENAME, _STORE_FILENAME + ) + + def snapshot(self) -> UserPreferences: + """Return a copy of the current preferences (sync; for the subscribe snapshot). + + A copy so a caller mutating it can't corrupt the canonical RAM state + (which would skip the debounced write and be lost on restart). + """ + return self._copy() + + def update( + self, fields: dict[str, Any], *, delay: float = _DEFAULT_SAVE_DELAY + ) -> UserPreferences: + """Merge a validated partial dict and schedule a debounced save.""" + self._state = UserPreferences.from_dict({**self._state.to_dict(), **fields}) + self._schedule_save(delay=delay) + return self._copy() + + def mutate( + self, + fn: Callable[[UserPreferences], UserPreferences | None], + *, + delay: float = _DEFAULT_SAVE_DELAY, + ) -> UserPreferences: + """Apply *fn* to a copy, replace RAM, schedule a save, return the result. + + *fn* may mutate the passed copy in place and return ``None`` (in-RAM + state is always replaced, never mutated in place, so a borrowed + :meth:`snapshot` reference stays stable). + """ + working = self._copy() + result = fn(working) + if result is None: + result = working + self._state = result + self._schedule_save(delay=delay) + return self._copy() + + def _schedule_save(self, *, delay: float) -> None: + """Schedule a debounced write, unless persistence has been disabled.""" + if self._persist_disabled: + return + self._store.async_delay_save(self._snapshot, delay=delay) + + def _copy(self) -> UserPreferences: + """Return a fresh, independent copy of the canonical RAM state.""" + return UserPreferences.from_dict(self._state.to_dict()) + + def _snapshot(self) -> UserPreferences: + return self._state + + def _preserve_corrupt_file(self) -> None: + """Rename the undecodable dedicated file aside so the next save can't erase it. + + If the rename fails, disable persistence: leaving the corrupt file in + place and then writing over it would destroy the recoverable data this + method exists to protect. + """ + path = self._config_dir / _STORE_FILENAME + try: + path.replace(path.with_name(path.name + ".corrupt")) + except OSError: + self._persist_disabled = True + _LOGGER.warning( + "Could not preserve corrupt preferences file %s; disabling writes to keep it", + path, + exc_info=True, + ) + + def _migrate_read_shared_sync(self) -> UserPreferences | None: + """Decode the sidecar's ``_preferences`` blob. + + Returns ``None`` when the key is absent or undecodable; an undecodable + legacy blob is logged and left in the sidecar (the caller doesn't strip + it) so the data stays recoverable rather than being replaced by defaults. + """ + shared_path = self._config_dir / _SHARED_SIDECAR_FILENAME + if not shared_path.exists(): + return None + data = _load_metadata(self._config_dir) + if _PREFS_KEY not in data: + return None + try: + return UserPreferences.from_dict(data[_PREFS_KEY]) + except _DECODE_ERRORS: + _LOGGER.exception( + "preferences store: legacy _preferences blob undecodable; left in %s for recovery", + _SHARED_SIDECAR_FILENAME, + ) + return None + + def _confirm_and_strip_shared_sync(self) -> bool: + """Strip the migrated ``_preferences`` key, but only if the write landed. + + ``Store`` logs and swallows write errors, so an unconfirmed flush leaves + the sidecar key for the next start to retry. The ``exists`` probe and the + strip share one executor hop to keep the blocking I/O off the loop. + """ + if not self._store.path.exists(): + return False + with metadata_transaction(self._config_dir) as data: + data.pop(_PREFS_KEY, None) + return True diff --git a/esphome_device_builder/controllers/config/controller.py b/esphome_device_builder/controllers/config/controller.py index 55ad55973..5ecad12c9 100644 --- a/esphome_device_builder/controllers/config/controller.py +++ b/esphome_device_builder/controllers/config/controller.py @@ -18,15 +18,16 @@ read_secrets_yaml, write_secret, ) +from ...helpers.storage import ShutdownCallback from ...helpers.storage_path import resolve_storage_path from ...models import ErrorCode, UserPreferences +from ._preferences_store import PreferencesStore from .chip_detect import ( _detect_chip_via_esptool, _detect_failure_message, _is_valid_port_name, _read_app_descriptor_board_id, ) -from .preferences import load_preferences, update_preferences if TYPE_CHECKING: from ...device_builder import DeviceBuilder @@ -37,6 +38,19 @@ class ConfigController: def __init__(self, device_builder: DeviceBuilder) -> None: self._db = device_builder + self._shutdown_callbacks: list[ShutdownCallback] = [] + self.prefs = PreferencesStore( + device_builder.settings.config_dir, self._shutdown_callbacks.append + ) + + async def async_load(self) -> None: + """Seed the RAM-canonical preferences store (and migrate on first run).""" + await self.prefs.async_load() + + async def stop(self) -> None: + """Flush the preferences store on shutdown.""" + for callback in self._shutdown_callbacks: + await callback() @api_command("config/version") async def get_version(self, **kwargs: Any) -> dict: @@ -94,9 +108,8 @@ async def detect_chip_cmd(self, **kwargs: Any) -> dict: @api_command("config/get_preferences") async def get_prefs(self, **kwargs: Any) -> UserPreferences: - """Get user preferences.""" - loop = asyncio.get_running_loop() - return await loop.run_in_executor(None, load_preferences, self._db.settings.config_dir) + """Get user preferences (RAM-canonical via the store).""" + return self.prefs.snapshot() @api_command("config/set_preferences") async def set_prefs(self, **kwargs: Any) -> UserPreferences: @@ -105,10 +118,8 @@ async def set_prefs(self, **kwargs: Any) -> UserPreferences: Accepts partial updates — only provided fields are changed, others keep their current values. """ - loop = asyncio.get_running_loop() - config_dir = self._db.settings.config_dir update_fields = {k: v for k, v in kwargs.items() if k not in ("client", "message_id")} - return await loop.run_in_executor(None, update_preferences, config_dir, update_fields) + return self.prefs.update(update_fields) @api_command("config/get_secrets") async def get_secrets(self, **kwargs: Any) -> list[str]: diff --git a/esphome_device_builder/controllers/config/preferences.py b/esphome_device_builder/controllers/config/preferences.py index 6e6a6e763..862c25cae 100644 --- a/esphome_device_builder/controllers/config/preferences.py +++ b/esphome_device_builder/controllers/config/preferences.py @@ -1,58 +1,5 @@ -"""User-preferences persistence backed by the metadata sidecar.""" +"""Legacy sidecar key the preferences migration reads from.""" from __future__ import annotations -from collections.abc import Callable -from pathlib import Path -from typing import Any - -from ...models import UserPreferences -from .metadata import _load_metadata, metadata_transaction - _PREFS_KEY = "_preferences" - - -def _prefs_from_data(data: dict[str, Any]) -> UserPreferences: - """Decode the ``_preferences`` blob, returning defaults on a corrupt shape.""" - try: - return UserPreferences.from_dict(data.get(_PREFS_KEY, {})) - except (ValueError, TypeError, LookupError): - return UserPreferences() - - -def load_preferences(config_dir: Path) -> UserPreferences: - """Load user preferences, returning defaults for missing or corrupt fields.""" - return _prefs_from_data(_load_metadata(config_dir)) - - -def save_preferences(config_dir: Path, prefs: UserPreferences) -> None: - """Save user preferences to disk.""" - with metadata_transaction(config_dir) as data: - data[_PREFS_KEY] = prefs.to_dict() - - -def mutate_preferences( - config_dir: Path, mutate: Callable[[UserPreferences], UserPreferences | None] -) -> UserPreferences: - """ - Atomic read-modify-write for user preferences. - - *mutate* receives the current prefs (defaults on a corrupt - blob) and returns the next state, or mutates it in place and - returns ``None``. Load and save share one lock. - """ - with metadata_transaction(config_dir) as data: - prefs = _prefs_from_data(data) - result = mutate(prefs) - if result is None: - result = prefs - data[_PREFS_KEY] = result.to_dict() - return result - - -def update_preferences(config_dir: Path, update_fields: dict[str, Any]) -> UserPreferences: - """Merge a validated partial dict into stored preferences atomically.""" - return mutate_preferences( - config_dir, - lambda prefs: UserPreferences.from_dict({**prefs.to_dict(), **update_fields}), - ) diff --git a/esphome_device_builder/controllers/onboarding.py b/esphome_device_builder/controllers/onboarding.py index c0322527c..4b484e46b 100644 --- a/esphome_device_builder/controllers/onboarding.py +++ b/esphome_device_builder/controllers/onboarding.py @@ -20,7 +20,6 @@ from __future__ import annotations import asyncio -from pathlib import Path from typing import TYPE_CHECKING, Any from ..helpers.api import CommandError, api_command @@ -38,9 +37,9 @@ UserPreferences, ) from ..models.onboarding import ONBOARDING_VERSION -from .config import load_preferences, mutate_preferences if TYPE_CHECKING: + from esphome_device_builder.controllers.config._preferences_store import PreferencesStore from esphome_device_builder.device_builder import DeviceBuilder @@ -58,6 +57,11 @@ class OnboardingController: def __init__(self, db: DeviceBuilder) -> None: self._db = db + @property + def _prefs(self) -> PreferencesStore: + assert self._db.config is not None + return self._db.config.prefs + @api_command("onboarding/get_state") async def get_state(self, **kwargs: Any) -> OnboardingState: """ @@ -69,9 +73,8 @@ async def get_state(self, **kwargs: Any) -> OnboardingState: wizard (any pending step OR new version available). """ loop = asyncio.get_running_loop() - secrets, prefs = await loop.run_in_executor( - None, _read_secrets_and_prefs, self._db.settings.config_dir - ) + secrets = await loop.run_in_executor(None, read_secrets_yaml, self._db.settings.config_dir) + prefs = self._prefs.snapshot() return OnboardingState( current_version=ONBOARDING_VERSION, @@ -161,8 +164,6 @@ async def mark_acknowledged(self, **kwargs: Any) -> OnboardingState: releases that add new steps bump that constant; existing users with a lower stored value will be re-prompted. """ - loop = asyncio.get_running_loop() - config_dir = self._db.settings.config_dir def _bump(prefs: UserPreferences) -> None: # max(), not assign: a rollback from a future build must @@ -171,16 +172,5 @@ def _bump(prefs: UserPreferences) -> None: prefs.onboarding_completed_version, ONBOARDING_VERSION ) - await loop.run_in_executor(None, mutate_preferences, config_dir, _bump) + self._prefs.mutate(_bump) return await self.get_state() - - -def _read_secrets_and_prefs(config_dir: Path) -> tuple[dict | None, UserPreferences]: - """ - Read both ``secrets.yaml`` and user preferences in one executor hop. - - Both are quick disk reads from the same config dir, so a single - executor job is cheaper than two. ``get_state`` runs on every - page load + after every secrets save, so the saved hop matters. - """ - return read_secrets_yaml(config_dir), load_preferences(config_dir) diff --git a/tests/controllers/config/__init__.py b/tests/controllers/config/__init__.py new file mode 100644 index 000000000..6ed08b589 --- /dev/null +++ b/tests/controllers/config/__init__.py @@ -0,0 +1 @@ +"""Tests for the config controller.""" diff --git a/tests/controllers/config/test_preferences_store.py b/tests/controllers/config/test_preferences_store.py new file mode 100644 index 000000000..8d03742be --- /dev/null +++ b/tests/controllers/config/test_preferences_store.py @@ -0,0 +1,264 @@ +"""Tests for ``PreferencesStore`` — RAM-canonical prefs + sidecar migration.""" + +from __future__ import annotations + +import asyncio +from pathlib import Path + +import orjson +import pytest + +from esphome_device_builder.controllers.config import _load_metadata, _save_metadata +from esphome_device_builder.controllers.config._preferences_store import ( + _STORE_FILENAME, + PreferencesStore, +) +from esphome_device_builder.models import UserPreferences +from esphome_device_builder.models.preferences import Theme + + +def _make_store(tmp_path: Path) -> PreferencesStore: + """Build a store anchored at *tmp_path* with a noop shutdown register.""" + return PreferencesStore(tmp_path, lambda _cb: None) + + +_SAMPLE = UserPreferences( + navigator_visible=False, + yaml_diff_button=True, + theme=Theme.DARK, +) + + +async def test_async_load_with_no_files_keeps_defaults(tmp_path: Path) -> None: + """Fresh install: no files, defaults in RAM, nothing written.""" + store = _make_store(tmp_path) + await store.async_load() + assert store.snapshot() == UserPreferences() + assert not (tmp_path / _STORE_FILENAME).exists() + assert not (tmp_path / ".device-builder.json").exists() + + +async def test_async_load_migrates_preferences_from_sidecar(tmp_path: Path) -> None: + """First run: ``_preferences`` moves to the dedicated file, other keys stay.""" + await asyncio.to_thread( + _save_metadata, + tmp_path, + { + "_preferences": _SAMPLE.to_dict(), + "_labels": [{"id": "abc", "name": "Bedroom"}], + "kitchen.yaml": {"board_id": "esp32"}, + }, + ) + store = _make_store(tmp_path) + await store.async_load() + + assert store.snapshot() == _SAMPLE + assert (tmp_path / _STORE_FILENAME).exists() + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert "_preferences" not in shared + assert shared["_labels"] == [{"id": "abc", "name": "Bedroom"}] + assert shared["kitchen.yaml"] == {"board_id": "esp32"} + + +async def test_async_load_leaves_corrupt_sidecar_blob_in_place(tmp_path: Path) -> None: + """A malformed sidecar ``_preferences`` blob is preserved, not migrated/stripped. + + Falling back to defaults *and* destroying the source would lose recoverable + data; instead leave the blob in the sidecar and don't write a dedicated file. + """ + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": [1, 2, 3]}) + store = _make_store(tmp_path) + await store.async_load() + + assert store.snapshot() == UserPreferences() + assert not (tmp_path / _STORE_FILENAME).exists() + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert shared["_preferences"] == [1, 2, 3] + + +async def test_async_load_keeps_sidecar_when_migration_write_unconfirmed( + tmp_path: Path, caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch +) -> None: + """A failed dedicated-file write during migration leaves the sidecar key to retry.""" + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + + def _boom(*_a: object, **_k: object) -> None: + raise OSError("disk full") + + monkeypatch.setattr("esphome_device_builder.helpers.storage.atomic_write", _boom) + store = _make_store(tmp_path) + with caplog.at_level("WARNING"): + await store.async_load() + + assert not (tmp_path / _STORE_FILENAME).exists() + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert shared["_preferences"] == _SAMPLE.to_dict() + assert any("write unconfirmed" in r.message for r in caplog.records) + + +async def test_failed_preserve_disables_writes_to_protect_corrupt_file( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """If the corrupt-file rename fails, writes are suppressed so it isn't clobbered.""" + raw = b"{not valid json" + (tmp_path / _STORE_FILENAME).write_bytes(raw) + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + + real_replace = Path.replace + + def _fail_corrupt_rename(self: Path, target: Path) -> Path: + if str(target).endswith(".corrupt"): + raise OSError("cannot rename") + return real_replace(self, target) + + monkeypatch.setattr(Path, "replace", _fail_corrupt_rename) + store = _make_store(tmp_path) + await store.async_load() + + # RAM recovered from the sidecar for this session, but nothing touches disk. + assert store.snapshot() == _SAMPLE + assert (tmp_path / _STORE_FILENAME).read_bytes() == raw + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert shared["_preferences"] == _SAMPLE.to_dict() + + # A later write stays suppressed; the recoverable corrupt file is never erased. + store.update({"theme": Theme.LIGHT}) + await store._store.async_save_now() + assert (tmp_path / _STORE_FILENAME).read_bytes() == raw + + +async def test_async_load_corrupt_dedicated_file_recovers_from_sidecar(tmp_path: Path) -> None: + """A corrupt dedicated file is preserved aside, then prefs recover from the sidecar.""" + (tmp_path / _STORE_FILENAME).write_bytes(b"{not valid json") + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + + store = _make_store(tmp_path) + await store.async_load() + + assert store.snapshot() == _SAMPLE + assert (tmp_path / (_STORE_FILENAME + ".corrupt")).read_bytes() == b"{not valid json" + assert (tmp_path / _STORE_FILENAME).exists() + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert "_preferences" not in shared + + +async def test_async_load_skips_migration_when_dedicated_file_exists(tmp_path: Path) -> None: + """An existing dedicated file wins; the sidecar ``_preferences`` is left alone.""" + new = UserPreferences(theme=Theme.LIGHT) + (tmp_path / _STORE_FILENAME).write_bytes(orjson.dumps(new.to_dict())) + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + + store = _make_store(tmp_path) + await store.async_load() + + assert store.snapshot() == new + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert shared["_preferences"] == _SAMPLE.to_dict() + + +async def test_async_load_no_preferences_key_keeps_defaults(tmp_path: Path) -> None: + """A sidecar with other keys but no ``_preferences`` migrates nothing.""" + await asyncio.to_thread(_save_metadata, tmp_path, {"kitchen.yaml": {"board_id": "esp32"}}) + store = _make_store(tmp_path) + await store.async_load() + + assert store.snapshot() == UserPreferences() + assert not (tmp_path / _STORE_FILENAME).exists() + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert shared["kitchen.yaml"] == {"board_id": "esp32"} + + +async def test_async_load_is_idempotent(tmp_path: Path) -> None: + """A second load reads the dedicated file; the sidecar stays as migration left it.""" + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + first = _make_store(tmp_path) + await first.async_load() + shared_after_first = await asyncio.to_thread(_load_metadata, tmp_path) + + second = _make_store(tmp_path) + await second.async_load() + assert second.snapshot() == first.snapshot() == _SAMPLE + assert await asyncio.to_thread(_load_metadata, tmp_path) == shared_after_first + + +async def test_async_load_preserves_corrupt_dedicated_file( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A corrupt dedicated file is renamed aside (not erased) and logged.""" + (tmp_path / _STORE_FILENAME).write_bytes(b"{not valid json") + store = _make_store(tmp_path) + with caplog.at_level("ERROR"): + await store.async_load() + assert store.snapshot() == UserPreferences() + assert any("undecodable" in r.message for r in caplog.records) + # Original preserved for recovery; the live file is gone so the next save + # can't overwrite a recoverable-but-corrupt read. + assert (tmp_path / (_STORE_FILENAME + ".corrupt")).read_bytes() == b"{not valid json" + assert not (tmp_path / _STORE_FILENAME).exists() + + +async def test_preserve_corrupt_file_logs_when_rename_fails( + tmp_path: Path, caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch +) -> None: + """A failed preservation rename is logged, not silently swallowed.""" + (tmp_path / _STORE_FILENAME).write_bytes(b"{not valid json") + + def _boom(self: Path, target: Path) -> None: + raise OSError("disk full") + + monkeypatch.setattr(Path, "replace", _boom) + store = _make_store(tmp_path) + with caplog.at_level("WARNING"): + await store.async_load() + assert store.snapshot() == UserPreferences() + assert any("could not preserve corrupt" in r.message.lower() for r in caplog.records) + + +async def test_async_load_preserves_non_dict_dedicated_file(tmp_path: Path) -> None: + """A dedicated file holding a non-object JSON value is preserved aside.""" + (tmp_path / _STORE_FILENAME).write_bytes(b"[1, 2, 3]") + store = _make_store(tmp_path) + await store.async_load() + assert store.snapshot() == UserPreferences() + assert (tmp_path / (_STORE_FILENAME + ".corrupt")).read_bytes() == b"[1, 2, 3]" + + +async def test_async_load_preserves_invalid_shape_dedicated_file(tmp_path: Path) -> None: + """A dedicated file whose object fails decode is preserved aside.""" + raw = b'{"theme": "bogus"}' + (tmp_path / _STORE_FILENAME).write_bytes(raw) + store = _make_store(tmp_path) + await store.async_load() + assert store.snapshot() == UserPreferences() + assert (tmp_path / (_STORE_FILENAME + ".corrupt")).read_bytes() == raw + + +async def test_mutators_return_independent_copies(tmp_path: Path) -> None: + """update()/mutate() return copies; mutating them can't corrupt canonical RAM.""" + store = _make_store(tmp_path) + await store.async_load() + + returned = store.update({"theme": Theme.DARK}) + returned.theme = Theme.LIGHT + assert store.snapshot().theme == Theme.DARK + + def _hide_nav(p: UserPreferences) -> None: + p.navigator_visible = False + + mutated = store.mutate(_hide_nav) + mutated.navigator_visible = True + assert store.snapshot().navigator_visible is False + + +async def test_round_trip_after_migration(tmp_path: Path) -> None: + """Migrate, mutate, flush, reload: the latest state survives on disk.""" + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + first = _make_store(tmp_path) + await first.async_load() + first.update({"theme": Theme.LIGHT}) + await first._store.async_save_now() + + second = _make_store(tmp_path) + await second.async_load() + assert second.snapshot().theme == Theme.LIGHT + assert second.snapshot().yaml_diff_button is True diff --git a/tests/test_config_controller.py b/tests/test_config_controller.py index 2a2cba6a0..5ac219cef 100644 --- a/tests/test_config_controller.py +++ b/tests/test_config_controller.py @@ -63,18 +63,16 @@ has_remote_build_settings_persisted, labels_transaction, load_labels, - load_preferences, load_remote_build_settings, metadata_transaction, remote_build_settings_transaction, remove_device_metadata, save_labels, - save_preferences, save_remote_build_settings, set_device_labels, set_device_metadata, - update_preferences, ) +from esphome_device_builder.controllers.config._preferences_store import PreferencesStore from esphome_device_builder.helpers.api import CommandError from esphome_device_builder.helpers.secrets_state import read_secrets_yaml from esphome_device_builder.models import ( @@ -95,8 +93,8 @@ from .conftest import MakeSettingsFactory, wire_secrets_writer -def _make_controller(config_dir: Path) -> ConfigController: - """Bypass __init__ chains; attach a stub DeviceBuilder.settings.""" +def _make_controller(config_dir: Path, *, prefs: UserPreferences | None = None) -> ConfigController: + """Bypass __init__ chains; attach a stub DeviceBuilder.settings + prefs store.""" controller = ConfigController.__new__(ConfigController) controller._db = MagicMock() controller._db.settings.config_dir = config_dir @@ -104,6 +102,11 @@ def _make_controller(config_dir: Path) -> ConfigController: controller._db.settings.rel_path = config_dir.joinpath controller._db.secrets_write_lock = asyncio.Lock() wire_secrets_writer(controller._db) + # RAM-canonical prefs store seeded in RAM (the debounce timer never fires + # within the test); get/set_prefs read and mutate the snapshot. + controller._shutdown_callbacks = [] + controller.prefs = PreferencesStore(config_dir, controller._shutdown_callbacks.append) + controller.prefs._state = prefs if prefs is not None else UserPreferences() return controller @@ -458,33 +461,6 @@ def test_remove_device_metadata_clears_only_target(tmp_path: Path) -> None: assert get_device_metadata(tmp_path, "b.yaml") == {"board_id": "esp8266"} -def test_load_preferences_returns_defaults_on_missing(tmp_path: Path) -> None: - """A fresh install has no preferences — fall back to the default object. - - Equality check (not just ``isinstance``) so a regression - that builds a non-default preferences object on the - missing-file path (e.g. one with ``dashboard_view=TABLE`` - instead of CARDS) breaks this test. - """ - assert load_preferences(tmp_path) == UserPreferences() - - -def test_load_preferences_returns_defaults_on_bad_data(tmp_path: Path) -> None: - """Corrupted preferences blob → default object, not partial recovery. - - ``UserPreferences.from_dict`` raises on unknown / malformed - fields; without the except-fallback the dashboard wouldn't - load when an older version's preferences file is read by a - newer mashumaro schema. Equality with ``UserPreferences()`` - pins that the fallback is the same default object, not a - silently-mutated one a regression could produce. - """ - metadata_path = tmp_path / ".device-builder.json" - metadata_path.write_bytes(b'{"_preferences": {"unknown_field": 42}}') - - assert load_preferences(tmp_path) == UserPreferences() - - def test_load_remote_build_settings_returns_defaults_on_missing(tmp_path: Path) -> None: """Fresh config dir → ``RemoteBuildSettings()`` with ``enabled=True``. @@ -731,89 +707,51 @@ def test_remote_build_settings_transaction_discards_on_exception( assert load_remote_build_settings(tmp_path) == RemoteBuildSettings(enabled=False) -def test_save_preferences_round_trip(tmp_path: Path) -> None: - """A non-default prefs blob round-trips through save → load. - - Pins the actual write path: round-tripping ``UserPreferences()`` - would also pass if save / load both silently lost data, so - use a non-default value (``dashboard_view=TABLE``) to - actually exercise the marshalling. - """ - prefs = UserPreferences(dashboard_view=DashboardView.TABLE) - save_preferences(tmp_path, prefs) - assert load_preferences(tmp_path) == prefs - - # --------------------------------------------------------------------------- # ConfigController WS commands — verifies file I/O runs off the event loop # --------------------------------------------------------------------------- -async def test_get_prefs_returns_loaded_preferences(tmp_path: Path) -> None: - """``get_prefs`` returns the persisted blob, not a fresh default. +async def test_config_controller_loads_and_flushes_preferences(tmp_path: Path) -> None: + """A real ``ConfigController`` builds the store, loads it, and flushes on stop.""" + db = MagicMock() + db.settings.config_dir = tmp_path + controller = ConfigController(db) # real __init__ builds the store + await controller.async_load() # no sidecar → defaults + assert controller.prefs.snapshot() == UserPreferences() + # A write schedules a debounced save; stop() flushes it to disk. + controller.prefs.update({"theme": Theme.DARK}) + await controller.stop() + assert (tmp_path / ".device-builder-preferences.json").exists() - Persists a non-default preferences object and asserts it - round-trips back. A regression that bypasses disk I/O and - just constructs ``UserPreferences()`` would still claim - ``isinstance`` but would fail this equality. - The seeding ``save_preferences`` runs in a thread because - ``metadata_transaction`` -> ``tempfile.mkstemp`` -> - ``os.path.abspath`` blocks the event loop, and blockbuster - flags it from inside an async test. - """ +async def test_get_prefs_returns_loaded_preferences(tmp_path: Path) -> None: + """``get_prefs`` returns the store's snapshot, not a fresh default.""" persisted = UserPreferences(dashboard_view=DashboardView.TABLE) - await asyncio.to_thread(save_preferences, tmp_path, persisted) - controller = _make_controller(tmp_path) + controller = _make_controller(tmp_path, prefs=persisted) prefs = await controller.get_prefs() assert prefs == persisted async def test_set_prefs_merges_partial_update(tmp_path: Path) -> None: - """Partial-update merge: only the supplied field changes. - - Persist a known initial state, then call ``set_prefs`` with - just one field. The unrelated fields must keep their - persisted values, not snap back to dataclass defaults — a - regression that re-constructs ``UserPreferences`` from - kwargs alone (skipping the merge step) would clobber them - silently. - - Seeding goes via ``asyncio.to_thread`` so the - ``metadata_transaction`` -> ``tempfile.mkstemp`` write - doesn't trip blockbuster on Linux CI. - """ + """Partial-update merge: only the supplied field changes; the rest survive.""" initial = UserPreferences(dashboard_view=DashboardView.TABLE, theme=Theme.DARK) - await asyncio.to_thread(save_preferences, tmp_path, initial) - controller = _make_controller(tmp_path) + controller = _make_controller(tmp_path, prefs=initial) # Update only ``theme``; ``dashboard_view`` should survive. result = await controller.set_prefs(theme=Theme.LIGHT) assert result.theme == Theme.LIGHT assert result.dashboard_view == DashboardView.TABLE - # Persisted blob matches the merged state. - persisted = await asyncio.to_thread(load_preferences, tmp_path) - assert persisted == result - - -def test_update_preferences_defaults_on_corrupt(tmp_path: Path) -> None: - """A malformed ``_preferences`` blob merges onto defaults, then persists clean.""" - metadata_path = tmp_path / ".device-builder.json" - metadata_path.write_bytes(b'{"_preferences": [1, 2, 3]}') - - updated = update_preferences(tmp_path, {"theme": Theme.DARK}) - - assert updated == UserPreferences(theme=Theme.DARK) - assert load_preferences(tmp_path) == UserPreferences(theme=Theme.DARK) + # The snapshot reflects the merged state. + assert controller.prefs.snapshot() == result async def test_set_prefs_concurrent_updates_do_not_lose_writes(tmp_path: Path) -> None: - """Concurrent partial updates of distinct fields all survive. + """Partial updates of distinct fields all survive on the RAM-canonical store. - The read-merge-save runs inside one ``metadata_transaction``, - so two writers can't both read the same baseline and clobber - each other's field. A non-atomic load-then-save loses one. + Each ``set_prefs`` merges onto the current snapshot in RAM, so distinct + fields accumulate rather than clobbering each other. """ controller = _make_controller(tmp_path) @@ -823,10 +761,10 @@ async def test_set_prefs_concurrent_updates_do_not_lose_writes(tmp_path: Path) - controller.set_prefs(navigator_visible=False), ) - persisted = await asyncio.to_thread(load_preferences, tmp_path) - assert persisted.theme == Theme.DARK - assert persisted.dashboard_view == DashboardView.TABLE - assert persisted.navigator_visible is False + snap = controller.prefs.snapshot() + assert snap.theme == Theme.DARK + assert snap.dashboard_view == DashboardView.TABLE + assert snap.navigator_visible is False async def test_get_secrets_returns_empty_when_missing(tmp_path: Path) -> None: diff --git a/tests/test_onboarding_controller.py b/tests/test_onboarding_controller.py index 767919186..302ffa1ca 100644 --- a/tests/test_onboarding_controller.py +++ b/tests/test_onboarding_controller.py @@ -15,11 +15,7 @@ import pytest -from esphome_device_builder.controllers.config import ( - load_preferences, - save_preferences, - update_preferences, -) +from esphome_device_builder.controllers.config._preferences_store import PreferencesStore from esphome_device_builder.controllers.onboarding import ( OnboardingController, ) @@ -39,13 +35,20 @@ from .conftest import wire_secrets_writer -def _make_controller(config_dir: Path) -> OnboardingController: +def _make_controller( + config_dir: Path, *, prefs: UserPreferences | None = None +) -> OnboardingController: controller = OnboardingController.__new__(OnboardingController) controller._db = MagicMock() controller._db.settings.config_dir = config_dir controller._db.settings.absolute_config_dir = config_dir.resolve() controller._db.secrets_write_lock = asyncio.Lock() wire_secrets_writer(controller._db) + # RAM-canonical prefs store seeded in RAM; mutations stay in RAM (the + # debounce timer never fires within the test), asserted via the snapshot. + store = PreferencesStore(config_dir, lambda _cb: None) + store._state = prefs if prefs is not None else UserPreferences() + controller._db.config.prefs = store return controller @@ -224,9 +227,7 @@ async def test_mark_acknowledged_persists_current_version(tmp_path: Path) -> Non controller = _make_controller(tmp_path) state = await controller.mark_acknowledged() assert state.completed_version == ONBOARDING_VERSION - # Re-read on a fresh controller to confirm the prefs file landed. - state2 = await _make_controller(tmp_path).get_state() - assert state2.completed_version == ONBOARDING_VERSION + assert controller._prefs.snapshot().onboarding_completed_version == ONBOARDING_VERSION async def test_mark_acknowledged_is_idempotent(tmp_path: Path) -> None: @@ -236,25 +237,13 @@ async def test_mark_acknowledged_is_idempotent(tmp_path: Path) -> None: assert state.completed_version == ONBOARDING_VERSION -async def test_mark_acknowledged_does_not_clobber_a_concurrent_pref_write( - tmp_path: Path, -) -> None: - """Concurrent acknowledgement and a ``set_preferences`` write keep both fields. - - Both share the ``_preferences`` blob and go through - ``metadata_transaction``, so neither can read a stale baseline - and overwrite the other's field. - """ - controller = _make_controller(tmp_path) - - await asyncio.gather( - controller.mark_acknowledged(), - asyncio.to_thread(update_preferences, tmp_path, {"theme": Theme.DARK}), - ) - - persisted = await asyncio.to_thread(load_preferences, tmp_path) - assert persisted.onboarding_completed_version == ONBOARDING_VERSION - assert persisted.theme == Theme.DARK +async def test_mark_acknowledged_keeps_other_pref_fields(tmp_path: Path) -> None: + """Acknowledging touches only the version, leaving an unrelated field intact.""" + controller = _make_controller(tmp_path, prefs=UserPreferences(theme=Theme.DARK)) + await controller.mark_acknowledged() + snap = controller._prefs.snapshot() + assert snap.onboarding_completed_version == ONBOARDING_VERSION + assert snap.theme == Theme.DARK async def test_mark_acknowledged_does_not_downgrade_a_higher_stored_version( @@ -262,19 +251,14 @@ async def test_mark_acknowledged_does_not_downgrade_a_higher_stored_version( ) -> None: """Don't lose a future-build acknowledgement on rollback. - A user who briefly ran a future build with - ``ONBOARDING_VERSION = 2`` and then rolled back to this - build (``= 1``) keeps the higher stored value — otherwise - they'd be re-prompted on the next upgrade for steps they've - already done. + A user who briefly ran a future build with a higher + ``ONBOARDING_VERSION`` and then rolled back keeps the higher stored + value — otherwise they'd be re-prompted on the next upgrade for steps + they've already done. """ - future = UserPreferences(onboarding_completed_version=ONBOARDING_VERSION + 5) - # ``save_preferences`` does sync filesystem I/O that ``blockbuster`` - # rejects when called inline from an async test. Hop to an executor - # so we behave like the controller does in production. - loop = asyncio.get_running_loop() - await loop.run_in_executor(None, save_preferences, tmp_path, future) - controller = _make_controller(tmp_path) + controller = _make_controller( + tmp_path, prefs=UserPreferences(onboarding_completed_version=ONBOARDING_VERSION + 5) + ) state = await controller.mark_acknowledged() assert state.completed_version == ONBOARDING_VERSION + 5 From b9bc112fbb0c5d6be367f7c3c7d178c0f31431af Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 15 Jun 2026 08:34:07 -0500 Subject: [PATCH 2/6] Deliver user preferences in the subscribe_events initial_state snapshot --- docs/API.md | 2 +- esphome_device_builder/device_builder.py | 12 +++++- tests/test_subscribe_events_cleanup.py | 53 +++++++++++++++++++++++- 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/docs/API.md b/docs/API.md index b1af37477..556f2329c 100644 --- a/docs/API.md +++ b/docs/API.md @@ -428,7 +428,7 @@ Same-subnet peers read `remote_build_port` from TXT so a `--remote-build-port` o **`subscribe_events` initial state:** -Right after a client subscribes (and before any live events arrive), the server pushes one `initial_state` event carrying a snapshot of state that's accumulated server-side via background activity (mDNS browser, completed pair flows, etc.) so the frontend can paint the first frame without follow-up reads. Shape: `{devices?: [...], importable?: [...], pairings?: [PairingSummary], peers?: [PeerSummary], hosts?: [RemoteBuildPeer], offloader_alerts?: [OffloaderAlertSnapshotEntry], peer_queue_status?: [PeerQueueStatusSnapshotEntry], remote_jobs?: [OffloaderRemoteJobSnapshotEntry], remote_builds_enabled?: bool, version_match_policy?: "any" | "release" | "exact" | "exact_required"}`. Each field is present only when the corresponding controller is up; `pairings` carries both PENDING and APPROVED offloader-side rows from the `_pairings` dict, `peers` carries both PENDING (`_pending_peers`) and APPROVED (`_approved_peers`) receiver-side rows, `hosts` carries the receiver controller's mDNS-discovered peer dashboards (`self._peers`, RAM-only — never persisted), `offloader_alerts` carries the offloader-side pair alerts dict (`_offloader_alerts`, RAM-only) so a tab subscribing AFTER a `pin_mismatch` / `peer_revoked` event fired still renders the alert it would have missed on the live stream — the alert only clears via re-pair or unpair, never by an operator-driven dismiss, because the underlying state (broken pairing) doesn't fix itself. `peer_queue_status` carries the most recent `queue_status` snapshot per paired receiver so a late tab paints the per-peer queue depth without waiting for the next event. `remote_jobs` carries every offloader-submitted job that's still in flight (terminal entries drop on the matching `job_state_changed` event) so the UI can render running builds on page load. `remote_builds_enabled` and `version_match_policy` carry the current value of the offloader-wide settings so the Settings dialog renders both controls on first paint instead of waiting for the matching `offloader_remote_builds_toggled` / `offloader_version_match_policy_changed` event to fire. All sync reads, no executor hop, no disk I/O. The `PeerSummary` projection persists `peer_ip` (the source IP observed at pair_request time) on `StoredPeer` so a snapshot-loaded inbox row carries the same IP the live `remote_build_pair_request_received` event would carry; that's what the receiver Settings UI renders alongside the pin as a clone-risk sanity-check. Empty string for legacy on-disk rows from receivers that pre-date the field. Live updates that arrive after the initial state mutate against this seed via the events below. +Right after a client subscribes (and before any live events arrive), the server pushes one `initial_state` event carrying a snapshot of state that's accumulated server-side via background activity (mDNS browser, completed pair flows, etc.) so the frontend can paint the first frame without follow-up reads. Shape: `{preferences: UserPreferences, devices?: [...], importable?: [...], pairings?: [PairingSummary], peers?: [PeerSummary], hosts?: [RemoteBuildPeer], offloader_alerts?: [OffloaderAlertSnapshotEntry], peer_queue_status?: [PeerQueueStatusSnapshotEntry], remote_jobs?: [OffloaderRemoteJobSnapshotEntry], remote_builds_enabled?: bool, version_match_policy?: "any" | "release" | "exact" | "exact_required"}`. `preferences` is always present — it's RAM-canonical behind a `PreferencesStore` (loaded once at startup, mutations debounce a write to its own `.device-builder-preferences.json`, migrated out of the shared sidecar on first run; the same per-file `Store` pattern as the device-metadata and peer-link stores), so the client paints theme/UI state without a separate `config/get_preferences`. Undecodable preferences are preserved, never destroyed: a corrupt dedicated file is renamed to `.corrupt` and an undecodable legacy sidecar blob is left in place (not stripped), both logged, before the store falls back to defaults. The rest are present only when the corresponding controller is up; `pairings` carries both PENDING and APPROVED offloader-side rows from the `_pairings` dict, `peers` carries both PENDING (`_pending_peers`) and APPROVED (`_approved_peers`) receiver-side rows, `hosts` carries the receiver controller's mDNS-discovered peer dashboards (`self._peers`, RAM-only — never persisted), `offloader_alerts` carries the offloader-side pair alerts dict (`_offloader_alerts`, RAM-only) so a tab subscribing AFTER a `pin_mismatch` / `peer_revoked` event fired still renders the alert it would have missed on the live stream — the alert only clears via re-pair or unpair, never by an operator-driven dismiss, because the underlying state (broken pairing) doesn't fix itself. `peer_queue_status` carries the most recent `queue_status` snapshot per paired receiver so a late tab paints the per-peer queue depth without waiting for the next event. `remote_jobs` carries every offloader-submitted job that's still in flight (terminal entries drop on the matching `job_state_changed` event) so the UI can render running builds on page load. `remote_builds_enabled` and `version_match_policy` carry the current value of the offloader-wide settings so the Settings dialog renders both controls on first paint instead of waiting for the matching `offloader_remote_builds_toggled` / `offloader_version_match_policy_changed` event to fire. All sync reads, no executor hop, no disk I/O. The `PeerSummary` projection persists `peer_ip` (the source IP observed at pair_request time) on `StoredPeer` so a snapshot-loaded inbox row carries the same IP the live `remote_build_pair_request_received` event would carry; that's what the receiver Settings UI renders alongside the pin as a clone-risk sanity-check. Empty string for legacy on-disk rows from receivers that pre-date the field. Live updates that arrive after the initial state mutate against this seed via the events below. **`subscribe_events` events:** - `device_added`, `device_removed`, `device_updated`, `device_state_changed` diff --git a/esphome_device_builder/device_builder.py b/esphome_device_builder/device_builder.py index ba5b07e8e..39e65c966 100644 --- a/esphome_device_builder/device_builder.py +++ b/esphome_device_builder/device_builder.py @@ -336,6 +336,9 @@ async def start(self) -> None: self.remote_build_offloader = OffloaderController(self) self.remote_build_receiver = ReceiverController(self) self.version_history = VersionHistoryController(self) + # Seed the RAM-canonical preferences (and migrate them out of the shared + # sidecar on first run) before onboarding reads or mutates them. + await self.config.async_load() await self.devices.start() await self.firmware.start() await self.editor.start() @@ -432,7 +435,7 @@ async def start(self) -> None: len(self.command_handlers), ) - async def stop(self) -> None: # noqa: C901 + async def stop(self) -> None: # noqa: C901, PLR0912 """Shut down the application.""" _LOGGER.info("Shutting down ESPHome Device Builder") if self._bg_task: @@ -470,6 +473,8 @@ async def stop(self) -> None: # noqa: C901 await self.editor.stop() if self.version_history is not None: await self.version_history.stop() + if self.config is not None: + await self.config.stop() # Cleanly drain the pool once nothing else can hand it work. # Two paths because the pool is created eagerly in ``__init__`` # — calling ``stop()`` on an instance that never ran @@ -592,6 +597,11 @@ async def _send_initial(_controls: StreamControls) -> None: # snapshot here a fresh page load would miss everything # the dashboard had already accumulated by then. initial: dict[str, Any] = {} + # Gate first-paint UI, so ship them here instead of a separate + # get_preferences round-trip. Sync RAM read off the store; the + # config controller is always up by the time subscribe is served. + if self.config is not None: + initial["preferences"] = self.config.prefs.snapshot().to_dict() if self.devices: initial["devices"] = [d.to_dict() for d in self.devices.get_devices()] initial["importable"] = [d.to_dict() for d in self.devices.get_importable_devices()] diff --git a/tests/test_subscribe_events_cleanup.py b/tests/test_subscribe_events_cleanup.py index 5723c1063..9bda6c176 100644 --- a/tests/test_subscribe_events_cleanup.py +++ b/tests/test_subscribe_events_cleanup.py @@ -26,11 +26,22 @@ StreamBackpressureError, ) from esphome_device_builder.helpers.subscriber_presence import SubscriberPresence -from esphome_device_builder.models import EventType +from esphome_device_builder.models import EventType, UserPreferences +from esphome_device_builder.models.preferences import Theme from .conftest import FakeWebSocketClient +def _stub_config(db: DeviceBuilder, prefs: UserPreferences | None = None) -> None: + """Give *db* a config controller whose prefs store returns *prefs*. + + ``_send_initial`` reads the snapshot off ``config.prefs``; default it to + plain defaults, which is all most of these stubs need. + """ + db.config = MagicMock() + db.config.prefs.snapshot.return_value = prefs or UserPreferences() + + def _make_db() -> DeviceBuilder: """Build a minimally-initialised DeviceBuilder for the handler. @@ -40,6 +51,7 @@ def _make_db() -> DeviceBuilder: stub. """ db = DeviceBuilder.__new__(DeviceBuilder) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None # skip the device-snapshot branch @@ -143,6 +155,7 @@ async def test_subscribe_events_includes_pairings_snapshot_in_initial_state() -> hop, no disk read. """ db = DeviceBuilder.__new__(DeviceBuilder) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None # skip the device-snapshot branch @@ -190,6 +203,7 @@ async def test_subscribe_events_includes_peers_snapshot_in_initial_state() -> No ``_pending_peers`` + ``_approved_peers`` dicts. """ db = DeviceBuilder.__new__(DeviceBuilder) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None @@ -247,6 +261,40 @@ async def test_subscribe_events_includes_peers_snapshot_in_initial_state() -> No await asyncio.gather(handler_task, return_exceptions=True) +async def test_subscribe_events_includes_preferences_in_initial_state() -> None: + """``_send_initial`` carries the UI-gating preferences in the snapshot. + + So first paint doesn't chase a separate ``get_preferences``. + """ + db = DeviceBuilder.__new__(DeviceBuilder) + _stub_config( + db, + UserPreferences(navigator_visible=False, theme=Theme.DARK), + ) + db.bus = EventBus() + db.subscriber_presence = SubscriberPresence() + db.devices = None + db.remote_build_offloader = None + db.remote_build_receiver = None + + client = FakeWebSocketClient() + handler_task = asyncio.create_task(db._cmd_subscribe_events(client=client, message_id="m1")) + for _ in range(50): + await asyncio.sleep(0) + if client.events: + break + + initial_events = [e for e in client.events if e[1] == "initial_state"] + assert len(initial_events) == 1 + _, _, payload = initial_events[0] + prefs = payload["preferences"] + assert prefs["navigator_visible"] is False + assert prefs["theme"] == "dark" + + handler_task.cancel() + await asyncio.gather(handler_task, return_exceptions=True) + + async def test_subscribe_events_includes_offloader_settings_in_initial_state() -> None: """``_send_initial`` merges the offloader-wide settings into the seed. @@ -257,6 +305,7 @@ async def test_subscribe_events_includes_offloader_settings_in_initial_state() - (or in the picker's case never, since there's nothing to flip). """ db = DeviceBuilder.__new__(DeviceBuilder) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None @@ -301,6 +350,7 @@ async def test_subscribe_events_includes_hosts_snapshot_in_initial_state() -> No command. """ db = DeviceBuilder.__new__(DeviceBuilder) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None @@ -405,6 +455,7 @@ async def test_subscribe_events_subscribed_arrives_before_live_events() -> None: must be queued, then drained strictly after the seed. """ db = DeviceBuilder.__new__(DeviceBuilder) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() From de4cb4429e2101fbd7595d706a8566107e76dc8d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 15 Jun 2026 08:53:49 -0500 Subject: [PATCH 3/6] Narrow config access with a raise, not an -O-strippable assert --- esphome_device_builder/controllers/onboarding.py | 5 ++++- esphome_device_builder/device_builder.py | 11 +++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/esphome_device_builder/controllers/onboarding.py b/esphome_device_builder/controllers/onboarding.py index 4b484e46b..96fd8d2e8 100644 --- a/esphome_device_builder/controllers/onboarding.py +++ b/esphome_device_builder/controllers/onboarding.py @@ -59,7 +59,10 @@ def __init__(self, db: DeviceBuilder) -> None: @property def _prefs(self) -> PreferencesStore: - assert self._db.config is not None + # config is created in start() before any onboarding command is served; + # raise (not assert, which -O strips) if that invariant is ever broken. + if self._db.config is None: + raise RuntimeError("config controller is not initialized") return self._db.config.prefs @api_command("onboarding/get_state") diff --git a/esphome_device_builder/device_builder.py b/esphome_device_builder/device_builder.py index 39e65c966..7afad91d8 100644 --- a/esphome_device_builder/device_builder.py +++ b/esphome_device_builder/device_builder.py @@ -598,10 +598,13 @@ async def _send_initial(_controls: StreamControls) -> None: # the dashboard had already accumulated by then. initial: dict[str, Any] = {} # Gate first-paint UI, so ship them here instead of a separate - # get_preferences round-trip. Sync RAM read off the store; the - # config controller is always up by the time subscribe is served. - if self.config is not None: - initial["preferences"] = self.config.prefs.snapshot().to_dict() + # get_preferences round-trip. Sync RAM read off the store. Always + # present per the wire contract; the config controller is created in + # start() before any subscribe is served, so raise (don't silently + # omit) if that invariant is ever broken. + if self.config is None: + raise RuntimeError("config controller is not initialized") + initial["preferences"] = self.config.prefs.snapshot().to_dict() if self.devices: initial["devices"] = [d.to_dict() for d in self.devices.get_devices()] initial["importable"] = [d.to_dict() for d in self.devices.get_importable_devices()] From 84ae3bb74925e99e847298bc00f2a2547856ac76 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 15 Jun 2026 08:53:57 -0500 Subject: [PATCH 4/6] Keep boot alive when stripping the migrated legacy key fails --- .../controllers/config/_preferences_store.py | 26 ++++++++++++++----- .../config/test_preferences_store.py | 26 +++++++++++++++++++ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py index 82d0aa40b..f824b195c 100644 --- a/esphome_device_builder/controllers/config/_preferences_store.py +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -200,14 +200,26 @@ def _migrate_read_shared_sync(self) -> UserPreferences | None: return None def _confirm_and_strip_shared_sync(self) -> bool: - """Strip the migrated ``_preferences`` key, but only if the write landed. - - ``Store`` logs and swallows write errors, so an unconfirmed flush leaves - the sidecar key for the next start to retry. The ``exists`` probe and the - strip share one executor hop to keep the blocking I/O off the loop. + """Strip the migrated ``_preferences`` key once the dedicated write landed. + + Returns ``False`` only when the dedicated-file write is unconfirmed + (``Store`` swallows write errors), so the caller keeps the legacy key for + a retry. A *strip* failure is non-fatal and still returns ``True``: the + dedicated file is already canonical and a leftover legacy key is benign + (the dedicated file wins on the next load), so it must not abort boot. + The ``exists`` probe and the strip share one executor hop. """ if not self._store.path.exists(): return False - with metadata_transaction(self._config_dir) as data: - data.pop(_PREFS_KEY, None) + try: + with metadata_transaction(self._config_dir) as data: + data.pop(_PREFS_KEY, None) + except OSError: + _LOGGER.warning( + "preferences store: migrated prefs but could not strip %s from %s; " + "the dedicated file wins, leaving the stale key", + _PREFS_KEY, + _SHARED_SIDECAR_FILENAME, + exc_info=True, + ) return True diff --git a/tests/controllers/config/test_preferences_store.py b/tests/controllers/config/test_preferences_store.py index 8d03742be..3ab605145 100644 --- a/tests/controllers/config/test_preferences_store.py +++ b/tests/controllers/config/test_preferences_store.py @@ -96,6 +96,32 @@ def _boom(*_a: object, **_k: object) -> None: assert any("write unconfirmed" in r.message for r in caplog.records) +async def test_migration_strip_failure_is_non_fatal( + tmp_path: Path, caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch +) -> None: + """A failed legacy-key strip after a confirmed write logs and completes, not aborts.""" + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + + def _boom(_config_dir: Path) -> None: + raise OSError("sidecar locked") + + monkeypatch.setattr( + "esphome_device_builder.controllers.config._preferences_store.metadata_transaction", + _boom, + ) + store = _make_store(tmp_path) + with caplog.at_level("WARNING"): + await store.async_load() + + # Migration still adopted the prefs and wrote the canonical dedicated file. + assert store.snapshot() == _SAMPLE + assert (tmp_path / _STORE_FILENAME).exists() + assert any("could not strip" in r.message.lower() for r in caplog.records) + # The strip failed, so the (now-benign) legacy key is left in place. + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert shared["_preferences"] == _SAMPLE.to_dict() + + async def test_failed_preserve_disables_writes_to_protect_corrupt_file( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: From daa93b9f421c191dd1def6a8a74686eb0fdeb021 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 15 Jun 2026 08:53:58 -0500 Subject: [PATCH 5/6] Collapse a test helper docstring to the single-line convention --- tests/test_subscribe_events_cleanup.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/test_subscribe_events_cleanup.py b/tests/test_subscribe_events_cleanup.py index 9bda6c176..9f293b50d 100644 --- a/tests/test_subscribe_events_cleanup.py +++ b/tests/test_subscribe_events_cleanup.py @@ -33,11 +33,7 @@ def _stub_config(db: DeviceBuilder, prefs: UserPreferences | None = None) -> None: - """Give *db* a config controller whose prefs store returns *prefs*. - - ``_send_initial`` reads the snapshot off ``config.prefs``; default it to - plain defaults, which is all most of these stubs need. - """ + """Give *db* a config controller whose prefs store snapshot returns *prefs*.""" db.config = MagicMock() db.config.prefs.snapshot.return_value = prefs or UserPreferences() From f0a948e6e297e5bacb9c2264702b8c87ace881bd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 15 Jun 2026 09:07:50 -0500 Subject: [PATCH 6/6] Exclude the always-up config guards from coverage --- esphome_device_builder/controllers/onboarding.py | 2 +- esphome_device_builder/device_builder.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/esphome_device_builder/controllers/onboarding.py b/esphome_device_builder/controllers/onboarding.py index 96fd8d2e8..59d6ecc5a 100644 --- a/esphome_device_builder/controllers/onboarding.py +++ b/esphome_device_builder/controllers/onboarding.py @@ -61,7 +61,7 @@ def __init__(self, db: DeviceBuilder) -> None: def _prefs(self) -> PreferencesStore: # config is created in start() before any onboarding command is served; # raise (not assert, which -O strips) if that invariant is ever broken. - if self._db.config is None: + if self._db.config is None: # pragma: no cover — config is always up post-start raise RuntimeError("config controller is not initialized") return self._db.config.prefs diff --git a/esphome_device_builder/device_builder.py b/esphome_device_builder/device_builder.py index 7afad91d8..0dc64881c 100644 --- a/esphome_device_builder/device_builder.py +++ b/esphome_device_builder/device_builder.py @@ -602,7 +602,7 @@ async def _send_initial(_controls: StreamControls) -> None: # present per the wire contract; the config controller is created in # start() before any subscribe is served, so raise (don't silently # omit) if that invariant is ever broken. - if self.config is None: + if self.config is None: # pragma: no cover — config is always up post-start raise RuntimeError("config controller is not initialized") initial["preferences"] = self.config.prefs.snapshot().to_dict() if self.devices: