From c9a50138b8d2a8c6307d8954d5b053d87f952e74 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 04:18:18 +0100 Subject: [PATCH 01/15] Remember the hostname a flash-free rename leaves on the device The config-only and in-place rename paths rewrite the YAML without flashing, so the firmware keeps broadcasting its pre-rename hostname while the dashboard forgets it. Record that name as deployed_name in the device-metadata store and publish its cached addresses under the new name's OTA cache key when the new name resolves nothing; clear it on the next landed flash, on a completed rename chain, or when a rename targets that name again. --- docs/API.md | 2 +- docs/ARCHITECTURE.md | 14 +++- .../controllers/devices/_metadata_store.py | 1 + .../controllers/devices/controller.py | 3 +- .../controllers/devices/firmware_sync.py | 6 ++ .../controllers/devices/helpers.py | 18 ++++- .../controllers/devices/mutations_simple.py | 22 +++++- .../devices/test_metadata_store.py | 1 + .../devices/test_rename_config_only.py | 47 ++++++++++++ .../devices/test_rename_metadata.py | 23 ++++++ .../firmware/test_address_cache.py | 72 ++++++++++++++++--- tests/controllers/firmware/test_refresh.py | 9 +++ 12 files changed, 199 insertions(+), 19 deletions(-) diff --git a/docs/API.md b/docs/API.md index 78a46fd00a..4f88c82237 100644 --- a/docs/API.md +++ b/docs/API.md @@ -126,7 +126,7 @@ Connections that arrive on the trusted ingress site (HA add-on supervisor proxy) | `devices/update` | `{configuration, friendly_name?, comment?, board_id?}` | `UpdateDeviceResponse` | Update device metadata (sidecar JSON) | | `devices/set_labels` | `{configuration, label_ids: string[]}` | `Device` | Replace this device's label assignments. Pass `[]` to clear. Unknown ids return `INVALID_ARGS`. Fires `device_updated` after the scanner reload when the assignments changed the row. | | `devices/set_labels_bulk` | `{updates: [{configuration, label_ids: string[]}, ...]}` | `[{configuration, success, error?}]` | Replace label assignments across multiple devices. One result per entry preserving input order (duplicates in `updates` produce duplicate rows). Per-entry failures (unknown id, missing device, malformed row) don't block the rest. Rows whose `configuration` can't be extracted (non-dict row, non-string value) surface as `{configuration: "", success: false, error}` so the frontend dialog can identify malformed entries from its own request payload. | -| `devices/rename` | `{configuration, new_name, config_only?: bool}` | `{configuration, job, tail_job}` (config-only / in-place: `{configuration, job: null}`) | Rename device. The online path writes the renamed YAML up-front and queues a **two-job chain**: `job` is the `COMPILE` of the new YAML (remote-eligible via the build scheduler, exactly like an install's) and `tail_job` is the dependent `RENAME` that OTA-flashes the *old* device address on the upload lane and swaps the files on success. A failed / cancelled chain deletes the new YAML so the old device is untouched. `config_only: true` (offline) and in-place renames rewrite the YAML with no jobs. A non-retargetable `esphome.name` (from packages / `!include` / an embedded substitution) refuses with `INVALID_ARGS` on every path. A config-only or in-place rename changes nothing and answers `PRECONDITION_FAILED` when the file changed while the rewrite was being validated, `NOT_FOUND` when it was deleted meanwhile, and the usual name-exists `INVALID_ARGS` when another config took the new filename meanwhile. | +| `devices/rename` | `{configuration, new_name, config_only?: bool}` | `{configuration, job, tail_job}` (config-only / in-place: `{configuration, job: null}`) | Rename device. The online path writes the renamed YAML up-front and queues a **two-job chain**: `job` is the `COMPILE` of the new YAML (remote-eligible via the build scheduler, exactly like an install's) and `tail_job` is the dependent `RENAME` that OTA-flashes the *old* device address on the upload lane and swaps the files on success. A failed / cancelled chain deletes the new YAML so the old device is untouched. `config_only: true` (offline) and in-place renames rewrite the YAML with no jobs. A non-retargetable `esphome.name` (from packages / `!include` / an embedded substitution) refuses with `INVALID_ARGS` on every path. Both record the pre-rename `esphome.name`, since the running firmware keeps broadcasting it until the next install, and the device's OTA address-cache args are then published under the new `.local` key using that name's cached addresses. A config-only or in-place rename changes nothing and answers `PRECONDITION_FAILED` when the file changed while the rewrite was being validated, `NOT_FOUND` when it was deleted meanwhile, and the usual name-exists `INVALID_ARGS` when another config took the new filename meanwhile. | | `devices/delete` | `{configuration}` | — | Delete device and associated files. A missing YAML surfaces `not_found` (same as `devices/archive`), not `internal_error`. | | `devices/delete_bulk` | `{configurations: string[]}` | `[{configuration, success, error?}]` | Delete multiple devices | | `devices/archive` | `{configuration}` | — | Soft-delete: move YAML to `/archive/`, wipe build dir, wipe StorageJSON + device-metadata sidecars. Reversible via `devices/unarchive` (cached IP/version/hash refill from the next mDNS broadcast). | diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 06ac4e0014..fbf651bdb4 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -264,7 +264,15 @@ firmware/install {configuration} → QUEUED → RUNNING → output... → COMPLE retry's supersede → write against the superseded chain's revert unlink; cancelling the tail cascades *up* to its compile. A persisted RENAME with no `depends_on` (pre-decomposition) still runs the fused - `esphome rename` CLI on the compile lane. + `esphome rename` CLI on the compile lane. The flash-free branches + (`config_only` for an offline device, and an in-place rename whose target + filename is the device's own) skip the chain, so the firmware keeps + broadcasting the pre-rename hostname. That name is recorded as + `deployed_name` in the device-metadata store and backs the device's OTA + address-cache args, so an install still reaches a device that never + announces its new name (#2730). A chained flash-free rename keeps the first + record, renaming back to it clears it, and it also clears on the next + landed flash or completed rename chain. - Plus a **remote build-server pool** — one more consumer (`run_dispatch_loop`) gathered alongside the lane workers. Compiles eligible for a paired server hold here (off the single compile lane) and run concurrently, one per @@ -803,7 +811,7 @@ The dashboard writes a small set of files into `` and `` a |---|---|---|---| | `.device-builder.json` | `` | Cross-flavor shared identity + per-device identity (`dashboard_id`, `_remote_build.enabled`, `_labels`; per-device `board_id` / `friendly_name` / `comment` / `labels` / `mac_address`). Shared across HA-addon flavors that mount the same `/config/esphome` tree. | umask default | | `.device-builder.json.corrupt`(`.`) | `` | Quarantined copies of an unparsable `.device-builder.json`, side-renamed by `metadata_transaction` before its write-back so the corrupt bytes stay recoverable. Original `.corrupt` is never overwritten; repeat incidents land at timestamped siblings pruned oldest-first to a cap of 3. Same sensitivity as the live sidecar. | inherits the sidecar's mode | -| `.device-builder-devices.json` | `` | Per-flavor live device state (`ip`, `expected_config_hash`, `deployed_config_hash`, `deployed_version`, `api_encryption_active`, `build_size_*`, `regen_failed_*`). Owned by `helpers.storage.Store` with debounced writes (2s coalesce); flushed on shutdown via the controller's `_shutdown_callbacks` list. | 0o600 enforced at write time (default for `Store`) | +| `.device-builder-devices.json` | `` | Per-flavor live device state (`ip`, `expected_config_hash`, `deployed_config_hash`, `deployed_version`, `deployed_name`, `api_encryption_active`, `build_size_*`, `regen_failed_*`). Owned by `helpers.storage.Store` with debounced writes (2s coalesce); flushed on shutdown via the controller's `_shutdown_callbacks` list. | 0o600 enforced at write time (default for `Store`) | | `.receiver_peers.json` | `` | Receiver-side pinned offloaders (`StoredPeer` rows: `(dashboard_id, pin_sha256, static_x25519_pub, label, paired_at, peer_ip, friendly_name, ha_addon, label_auto)`). Owned by `helpers.storage.Store` with debounced writes; only APPROVED rows ever reach disk (PENDING lives in `_pending_peers` and is bounded by the pairing window). A reader can enumerate which `dashboard_id`s have paired with this receiver, but neither pin nor pubkey is secret on its own. | 0o600 enforced at write time (default for `Store`) | | `.offloader_pairings.json` | `` | Offloader-side pinned receivers (`StoredPairing` rows: `(receiver_hostname, receiver_port, pin_sha256, static_x25519_pub, label, paired_at, status, esphome_version, enabled, auto_provision_supported, friendly_name, ha_addon, reset_build_env_supported, receiver_label_auto)`). Owned by `helpers.storage.Store` with debounced writes; only APPROVED rows ever reach disk (PENDING is filtered out at serialise time). Same secret-equivalent shape as the receiver's `.receiver_peers.json`: a reader can enumerate which receivers this offloader has paired with, but neither pin nor pubkey is secret on its own. | 0o600 enforced at write time (default for `Store`) | | `.device-builder-pending-keys.json` | `` | **Plaintext HA-provisioned Noise API keys awaiting adoption. Sensitive.** Name-keyed `{key, mac}` entries received over the ingress-only `POST /encryption-key`; consumed by `devices/import` and by later pushes once configured. A reader can connect to those devices' native APIs. Entries never expire by design — a stale entry is a cheaper failure than losing the only recovery copy of a key. | 0o600 enforced at write time (default for `Store`) | @@ -816,7 +824,7 @@ The dashboard writes a small set of files into `` and `` a Per-device metadata is partitioned across two files by *who writes it* and *how often*: * **Identity** (`board_id`, `friendly_name`, `comment`, `labels`, `mac_address`) lives in `/.device-builder.json` alongside the cross-flavor catalog keys (`_labels`, `_remote_build`, `dashboard_id`). Access goes through `SharedSidecarClient` — a thin async wrapper around the existing `helpers/metadata_sidecar.metadata_transaction` (`fcntl.flock` + `_METADATA_LOCK` for cross-flavor RMW safety). Writes are infrequent (user-edited names, scanner-derived `board_id` backfill, first-observation `mac_address`) and run through the transactional path so the `esphome` / `esphome-beta` / `esphome-dev` flavors on a shared `/config/esphome` can't clobber each other. -* **Live state** (`ip`, `expected_config_hash`, `deployed_config_hash`, `deployed_version`, `api_encryption_active`, `build_size_*`, `regen_failed_*`) lives in `/.device-builder-devices.json`. Access goes through `DeviceMetadataStore` — a `helpers.storage.Store`-backed RAM-canonical dict that debounces writes (2s coalesce) and flushes on shutdown. The store keys on `` rather than `` because each HA-addon flavor compiles its own binaries and observes its own mDNS broadcasts; sharing this state across flavors would let one flavor's running-firmware hash overwrite another's. The file is per-flavor by construction, so no cross-process lock is needed beyond the single-instance startup `flock` that already pins one process per `data_dir`. +* **Live state** (`ip`, `expected_config_hash`, `deployed_config_hash`, `deployed_version`, `deployed_name`, `api_encryption_active`, `build_size_*`, `regen_failed_*`) lives in `/.device-builder-devices.json`. Access goes through `DeviceMetadataStore` — a `helpers.storage.Store`-backed RAM-canonical dict that debounces writes (2s coalesce) and flushes on shutdown. The store keys on `` rather than `` because each HA-addon flavor compiles its own binaries and observes its own mDNS broadcasts; sharing this state across flavors would let one flavor's running-firmware hash overwrite another's. The file is per-flavor by construction, so no cross-process lock is needed beyond the single-instance startup `flock` that already pins one process per `data_dir`. The `STORE_FIELDS` frozenset in `controllers/devices/_metadata_store.py` enumerates the live-state field names; `DeviceMetadataBase._persist_device_metadata_async` is the routing dispatcher (anything in `STORE_FIELDS` → store, everything else → shared sidecar). The mDNS hot path (`state_callbacks.on_*`) writes the store directly via `controller._metadata_store.update(...)` / `set_field(...)` — sync RAM mutation on the event loop, debounced disk write on the executor. diff --git a/esphome_device_builder/controllers/devices/_metadata_store.py b/esphome_device_builder/controllers/devices/_metadata_store.py index 5b1dbd154d..f9bdd2b9c0 100644 --- a/esphome_device_builder/controllers/devices/_metadata_store.py +++ b/esphome_device_builder/controllers/devices/_metadata_store.py @@ -25,6 +25,7 @@ "ip", "deployed_config_hash", "deployed_version", + "deployed_name", "queued_update", "api_encryption_active", "expected_config_hash", diff --git a/esphome_device_builder/controllers/devices/controller.py b/esphome_device_builder/controllers/devices/controller.py index c3416b9507..2d7b96a4cd 100644 --- a/esphome_device_builder/controllers/devices/controller.py +++ b/esphome_device_builder/controllers/devices/controller.py @@ -428,7 +428,8 @@ def get_address_cache_args(self, configuration: str) -> list[str]: loaded = device.loaded_integrations if loaded and "api" not in loaded and "web_server" not in loaded: return [] - return _build_address_cache_args(device, self._state_monitor) + deployed_name = self._metadata_store.get(configuration).get("deployed_name", "") + return _build_address_cache_args(device, self._state_monitor, deployed_name) def get_ota_address_cache_args(self, configuration: str, port: str | None) -> list[str]: """Return cache args when ``port == "OTA"`` (or ``None`` for always-OTA flows).""" diff --git a/esphome_device_builder/controllers/devices/firmware_sync.py b/esphome_device_builder/controllers/devices/firmware_sync.py index 9ca18be6e4..ea2c6f579e 100644 --- a/esphome_device_builder/controllers/devices/firmware_sync.py +++ b/esphome_device_builder/controllers/devices/firmware_sync.py @@ -122,6 +122,9 @@ async def refresh_after_job( await controller._persist_expected_config_hash(configuration) await controller._scanner.reload(configuration) if flashed: + # The landed image carries the YAML's own name, so a pre-rename + # hostname recorded for it is spent. + controller._metadata_store.update(configuration, deployed_name="") await controller._sync_deployed_state_after_flash(configuration) controller._schedule_version_reprobe(configuration) # A real compile moves the build-size cache's freshness @@ -233,6 +236,9 @@ async def migrate_metadata_then_scan( controller: DevicesController, old_configuration: str, new_configuration: str ) -> None: """Move the renamed device's metadata before the scan rebuilds it.""" + # Same clear as the flashed branch of ``refresh_after_job``, which a + # completed RENAME never reaches. + controller._metadata_store.update(old_configuration, deployed_name="") await migrate_metadata(controller, old_configuration, new_configuration) await rescan_renamed(controller, new_configuration) diff --git a/esphome_device_builder/controllers/devices/helpers.py b/esphome_device_builder/controllers/devices/helpers.py index e073618198..764057047d 100644 --- a/esphome_device_builder/controllers/devices/helpers.py +++ b/esphome_device_builder/controllers/devices/helpers.py @@ -14,7 +14,7 @@ from ...helpers.async_ import run_in_executor from ...helpers.atomic_io import atomic_write_exclusive from ...helpers.device_config import raise_device_not_found -from ...helpers.hostname import is_local_hostname, normalize_hostname +from ...helpers.hostname import default_mdns_address, is_local_hostname, normalize_hostname from ...helpers.text import diff_excerpt, same_text from ...helpers.yaml import read_yaml_scalar, rewrite_name_or_substitution from ...models import ConfigEntryType, Device, ErrorCode @@ -476,8 +476,15 @@ def _gates_on_unconfigured_block( return bool(gate) and gate not in configured_blocks -def _build_address_cache_args(device: Device, monitor: DeviceStateMonitor | None) -> list[str]: - """Build CLI cache args from the IPs we already have for *device*.""" +def _build_address_cache_args( + device: Device, monitor: DeviceStateMonitor | None, deployed_name: str = "" +) -> list[str]: + """ + Build CLI cache args from the IPs we already have for *device*. + + *deployed_name* is the hostname the firmware still answers to after a + flash-free rename; its addresses back the YAML name's key. + """ address = device.address if not address: return [] @@ -504,6 +511,11 @@ def _build_address_cache_args(device: Device, monitor: DeviceStateMonitor | None ) if cached: addresses = list(cached) + if not addresses and is_local and deployed_name: + # Publish the pre-rename name's IPs under the key the CLI looks up. + stale = monitor.mdns.get_cached_addresses(default_mdns_address(deployed_name)) + if stale: + addresses = list(stale) if not addresses and device.ip: addresses = [device.ip] diff --git a/esphome_device_builder/controllers/devices/mutations_simple.py b/esphome_device_builder/controllers/devices/mutations_simple.py index 855a1615c8..3003935eac 100644 --- a/esphome_device_builder/controllers/devices/mutations_simple.py +++ b/esphome_device_builder/controllers/devices/mutations_simple.py @@ -244,7 +244,8 @@ def _read_and_probe() -> tuple[str, bool]: # a stem compare wrongly rejects the legitimate ``test_1`` -> ``test-1`` # rename and wrongly accepts a real no-op whose filename differs from its # name. - if new_name == resolved_device_name(old_meta, configuration): + old_name = resolved_device_name(old_meta, configuration) + if new_name == old_name: raise CommandError( ErrorCode.INVALID_ARGS, "new_name must differ from the current device name", @@ -276,6 +277,7 @@ def _read_and_probe() -> tuple[str, bool]: content=content, new_content=new_content, in_place=in_place, + old_name=old_name, ) firmware = controller._db.firmware @@ -296,13 +298,15 @@ async def _config_only_rename( content: str, new_content: str, in_place: bool, + old_name: str, ) -> dict[str, Any]: """ Land the rewritten YAML with no compile or OTA. Validates *new_content* before touching disk, writes the new file - atomically, removes the old, and migrates the StorageJSON + sidecar - metadata. Refuses with ``PRECONDITION_FAILED`` when the file no longer + atomically, removes the old, records the pre-rename hostname, and + migrates the StorageJSON + sidecar metadata. Refuses with + ``PRECONDITION_FAILED`` when the file no longer holds *content*, and never replaces a target another writer created. Returns ``job: None`` (nothing is queued). When *in_place* the target filename is the device's own file: the rewrite lands on it @@ -335,11 +339,23 @@ def _land() -> None: for name in sorted({os.path.normpath(n) for n in (configuration, new_filename)}): await locks.enter_async_context(controller._yaml_write_lock(name)) await run_in_executor(_land) + # Before the migrate, so its immediate flush is the only store write. + _stamp_deployed_name(controller, configuration, old_name=old_name, new_name=new_name) await migrate_metadata(controller, configuration, new_filename) await rescan_renamed(controller, new_filename) return {"configuration": new_filename, "job": None} +def _stamp_deployed_name( + controller: DevicesController, configuration: str, *, old_name: str, new_name: str +) -> None: + """Record the hostname the firmware still answers to after a flash-free rename.""" + stamped = controller._metadata_store.get(configuration).get("deployed_name") or old_name + controller._metadata_store.update( + configuration, deployed_name="" if stamped == new_name else stamped + ) + + def _migrate_storage_json(old_configuration: str, new_filename: str, new_name: str) -> None: """Move the StorageJSON sidecar to the new filename, retargeting the name. diff --git a/tests/controllers/devices/test_metadata_store.py b/tests/controllers/devices/test_metadata_store.py index ec2ae2fc55..6fdd0c1b35 100644 --- a/tests/controllers/devices/test_metadata_store.py +++ b/tests/controllers/devices/test_metadata_store.py @@ -612,6 +612,7 @@ def test_store_fields_pinned() -> None: "ip", "deployed_config_hash", "deployed_version", + "deployed_name", "queued_update", "api_encryption_active", "expected_config_hash", diff --git a/tests/controllers/devices/test_rename_config_only.py b/tests/controllers/devices/test_rename_config_only.py index b785076ab6..6c82045368 100644 --- a/tests/controllers/devices/test_rename_config_only.py +++ b/tests/controllers/devices/test_rename_config_only.py @@ -446,3 +446,50 @@ async def test_config_only_rename_still_rejects_collision( assert excinfo.value.code == ErrorCode.INVALID_ARGS assert "already exists" in excinfo.value.message + + +# ---------------------------------------------------------------------- +# deployed_name: the firmware keeps its old hostname until a flash (#2730) +# ---------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("text", "configuration", "renames", "expected_filename", "expected_deployed"), + [ + pytest.param(_YAML, "kitchen.yaml", ["livingroom"], "livingroom.yaml", "kitchen", id="one"), + # ``esphome.name`` is recorded, which the filename stem can differ from. + pytest.param( + _UNDERSCORE_YAML, "test-1.yaml", ["test-1"], "test-1.yaml", "test_1", id="in_place" + ), + # A second flash-free rename still points at what the firmware has. + pytest.param( + _YAML, "kitchen.yaml", ["livingroom", "hallway"], "hallway.yaml", "kitchen", id="chain" + ), + # Renaming back to it leaves nothing to redirect. + pytest.param( + _YAML, "kitchen.yaml", ["livingroom", "kitchen"], "kitchen.yaml", None, id="back" + ), + ], +) +async def test_config_only_rename_records_the_deployed_name( + tmp_path: Path, + make_controller: MakeControllerFactory, + text: str, + configuration: str, + renames: list[str], + expected_filename: str, + expected_deployed: str | None, +) -> None: + """The hostname the firmware still answers to is recorded under the new filename.""" + controller = make_controller(tmp_path) + (tmp_path / configuration).write_text(text, encoding="utf-8") + + current = configuration + for new_name in renames: + result = await controller.rename_device( + configuration=current, new_name=new_name, config_only=True + ) + current = result["configuration"] + + assert current == expected_filename + assert controller._metadata_store.get(current).get("deployed_name") == expected_deployed diff --git a/tests/controllers/devices/test_rename_metadata.py b/tests/controllers/devices/test_rename_metadata.py index 0b423cf774..881f8bf655 100644 --- a/tests/controllers/devices/test_rename_metadata.py +++ b/tests/controllers/devices/test_rename_metadata.py @@ -164,6 +164,29 @@ async def test_completed_rename_migrates_metadata_then_scans( assert controller._scanner.calls == [("reload", "livingroom.yaml"), ("scan", False)] +async def test_completed_rename_clears_the_deployed_name_record( + tmp_path: Path, make_controller: MakeControllerFactory +) -> None: + """The tail flashed the new name, so a carried-over record is spent.""" + controller = make_controller(tmp_path) + controller._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + + scheduled: list[Any] = [] + controller._db.create_background_task = scheduled.append + + job = FirmwareJob( + job_id="abc123", + configuration="kitchen.yaml", + job_type=JobType.RENAME, + status=JobStatus.COMPLETED, + new_name="livingroom", + ) + firmware_sync.on_job_completed(controller, Event(EventType.JOB_COMPLETED, {"job": job})) + await scheduled[0] + + assert "deployed_name" not in controller._metadata_store.get("livingroom.yaml") + + async def test_completed_rename_normalizes_new_name_with_extension( tmp_path: Path, make_controller: MakeControllerFactory ) -> None: diff --git a/tests/controllers/firmware/test_address_cache.py b/tests/controllers/firmware/test_address_cache.py index bd8970cbb4..44bf8d9a0e 100644 --- a/tests/controllers/firmware/test_address_cache.py +++ b/tests/controllers/firmware/test_address_cache.py @@ -9,6 +9,7 @@ from __future__ import annotations +from types import SimpleNamespace from typing import Any from unittest.mock import MagicMock @@ -42,9 +43,8 @@ def _monitor( Both maps are keyed by hostname with normalize_hostname semantics so production-equivalent inputs like ``Kitchen.Local.`` hit the - same entry as ``kitchen.local``. The two test addresses - (``kitchen.local`` and ``esp.example.com``) cover every test - in this file. + same entry as ``kitchen.local``. Tests needing per-host values + build ``RecordingStateMonitor`` directly. """ return RecordingStateMonitor( cached_addresses=_seed(addresses), @@ -147,24 +147,66 @@ def test_multiple_cached_addresses_sorted() -> None: assert set(ips.split(",")) == {"192.168.1.50", "fe80::1234"} +# ---------------------------------------------------------------------- +# deployed_name: the hostname the firmware still answers to after a +# rename that didn't flash (issue #2730). +# ---------------------------------------------------------------------- + + +def test_deployed_name_cache_backs_the_yaml_name_key() -> None: + """The pre-rename name's cached IPs are published under the YAML name's key.""" + monitor = RecordingStateMonitor(cached_addresses={"asistente.local": ["192.168.1.50"]}) + args = _build_address_cache_args(_device(), monitor, "asistente") + assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.50"] + + +def test_deployed_name_not_consulted_when_own_name_resolves() -> None: + """A hit on the device's own name wins; the stale lookup never runs.""" + monitor = RecordingStateMonitor( + cached_addresses={"kitchen.local": ["192.168.1.50"], "asistente.local": ["10.0.0.1"]} + ) + args = _build_address_cache_args(_device(), monitor, "asistente") + assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.50"] + assert monitor.calls == [("get_cached_addresses", "kitchen.local")] + + +def test_deployed_name_miss_falls_back_to_device_ip() -> None: + """Neither name cached → the persisted IP still backs the key.""" + monitor = RecordingStateMonitor(cached_addresses={}) + args = _build_address_cache_args(_device(ip="192.168.1.99"), monitor, "asistente") + assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.99"] + + +def test_deployed_name_ignored_for_non_local_address() -> None: + """A renamed ``.local`` name is irrelevant to a DNS-resolved address.""" + monitor = RecordingStateMonitor(cached_addresses={"asistente.local": ["192.168.1.50"]}) + args = _build_address_cache_args(_device(address="esp.example.com"), monitor, "asistente") + assert args == [] + assert not any(call[0] == "get_cached_addresses" for call in monitor.calls) + + # ---------------------------------------------------------------------- # DevicesController.get_address_cache_args integration gate # ---------------------------------------------------------------------- -def _devices_controller_with(*devices: Device) -> Any: +def _devices_controller_with(*devices: Device, deployed_name: str = "") -> Any: """Build a thin DevicesController shell with a stubbed scanner + monitor. - ``get_address_cache_args`` only reads the scanner's - configuration-keyed lookup, the state monitor's cached-addresses - lookup, and the device's ``loaded_integrations`` field — keep the - rest of the controller out of the test surface. + ``get_address_cache_args`` reads the scanner's configuration-keyed + lookup, the state monitor's cached-addresses lookup, the metadata + store's ``deployed_name`` record, and the device's + ``loaded_integrations`` field — keep the rest of the controller out + of the test surface. """ controller = DevicesController.__new__(DevicesController) scanner = RecordingScanner() scanner.devices = list(devices) controller._scanner = scanner controller._state_monitor = _monitor(["192.168.1.50"]) + controller._metadata_store = SimpleNamespace( + get=lambda _configuration: {"deployed_name": deployed_name} + ) return controller @@ -272,6 +314,20 @@ def test_get_ota_address_cache_args_empty_for_missing_port() -> None: assert controller.get_ota_address_cache_args("kitchen.yaml", "") == [] +def test_get_address_cache_args_reads_the_deployed_name_record() -> None: + """The controller feeds the store's record into the cache-args build.""" + controller = _devices_controller_with( + _device(loaded_integrations=["api"]), deployed_name="asistente" + ) + controller._state_monitor = RecordingStateMonitor( + cached_addresses={"asistente.local": ["192.168.1.50"]} + ) + + args = controller.get_address_cache_args("kitchen.yaml") + + assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.50"] + + def test_get_ota_address_cache_args_none_is_always_ota() -> None: """``port=None`` skips the OTA gate — for always-OTA flows like ``rename``.""" controller = _devices_controller_with(_device()) diff --git a/tests/controllers/firmware/test_refresh.py b/tests/controllers/firmware/test_refresh.py index 00f9ef0b9e..c6e9ca9d28 100644 --- a/tests/controllers/firmware/test_refresh.py +++ b/tests/controllers/firmware/test_refresh.py @@ -377,11 +377,20 @@ async def _fake_compute(path: Path) -> str | None: controller._state_monitor = MagicMock() # The flashed branch arms a post-flash re-probe timer. controller._reprobe_timers = {} + controller._shutdown_callbacks = [] + controller._metadata_store = DeviceMetadataStore( + config_dir=tmp_path, + data_dir=tmp_path, + shutdown_register=controller._shutdown_callbacks.append, + ) + controller._metadata_store.update("kitchen.yaml", deployed_name="old-kitchen", delay=0.0) await controller._refresh_after_firmware_job("kitchen.yaml", recompute_hash=False, flashed=True) assert compute_calls == [] assert controller._scanner.calls == [("reload", "kitchen.yaml")] + # The landed image carries the YAML's own name. + assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") controller._cancel_reprobe_timers() From edf9e11dc2750192b0616e7a60165142531e7eae Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 04:46:38 +0100 Subject: [PATCH 02/15] Record a hand-edited name and drop a reclaimed one A direct esphome.name edit is the third flash-free rename, so stamp deployed_name off the scan's name change too. Drop the record when another config claims that name; whatever answers to it is no longer this device, and targeting it would OTA the wrong board. --- docs/API.md | 2 +- docs/ARCHITECTURE.md | 9 ++++-- .../controllers/devices/controller.py | 4 +++ .../controllers/devices/metadata.py | 10 ++++++ .../controllers/devices/mutations_simple.py | 12 +------ .../controllers/devices/scan_change.py | 5 +++ .../devices/test_branches_coverage.py | 31 ++++++++++++++++--- .../firmware/test_address_cache.py | 29 +++++++++++++++-- 8 files changed, 81 insertions(+), 21 deletions(-) diff --git a/docs/API.md b/docs/API.md index 4f88c82237..a4ceaec496 100644 --- a/docs/API.md +++ b/docs/API.md @@ -126,7 +126,7 @@ Connections that arrive on the trusted ingress site (HA add-on supervisor proxy) | `devices/update` | `{configuration, friendly_name?, comment?, board_id?}` | `UpdateDeviceResponse` | Update device metadata (sidecar JSON) | | `devices/set_labels` | `{configuration, label_ids: string[]}` | `Device` | Replace this device's label assignments. Pass `[]` to clear. Unknown ids return `INVALID_ARGS`. Fires `device_updated` after the scanner reload when the assignments changed the row. | | `devices/set_labels_bulk` | `{updates: [{configuration, label_ids: string[]}, ...]}` | `[{configuration, success, error?}]` | Replace label assignments across multiple devices. One result per entry preserving input order (duplicates in `updates` produce duplicate rows). Per-entry failures (unknown id, missing device, malformed row) don't block the rest. Rows whose `configuration` can't be extracted (non-dict row, non-string value) surface as `{configuration: "", success: false, error}` so the frontend dialog can identify malformed entries from its own request payload. | -| `devices/rename` | `{configuration, new_name, config_only?: bool}` | `{configuration, job, tail_job}` (config-only / in-place: `{configuration, job: null}`) | Rename device. The online path writes the renamed YAML up-front and queues a **two-job chain**: `job` is the `COMPILE` of the new YAML (remote-eligible via the build scheduler, exactly like an install's) and `tail_job` is the dependent `RENAME` that OTA-flashes the *old* device address on the upload lane and swaps the files on success. A failed / cancelled chain deletes the new YAML so the old device is untouched. `config_only: true` (offline) and in-place renames rewrite the YAML with no jobs. A non-retargetable `esphome.name` (from packages / `!include` / an embedded substitution) refuses with `INVALID_ARGS` on every path. Both record the pre-rename `esphome.name`, since the running firmware keeps broadcasting it until the next install, and the device's OTA address-cache args are then published under the new `.local` key using that name's cached addresses. A config-only or in-place rename changes nothing and answers `PRECONDITION_FAILED` when the file changed while the rewrite was being validated, `NOT_FOUND` when it was deleted meanwhile, and the usual name-exists `INVALID_ARGS` when another config took the new filename meanwhile. | +| `devices/rename` | `{configuration, new_name, config_only?: bool}` | `{configuration, job, tail_job}` (config-only / in-place: `{configuration, job: null}`) | Rename device. The online path writes the renamed YAML up-front and queues a **two-job chain**: `job` is the `COMPILE` of the new YAML (remote-eligible via the build scheduler, exactly like an install's) and `tail_job` is the dependent `RENAME` that OTA-flashes the *old* device address on the upload lane and swaps the files on success. A failed / cancelled chain deletes the new YAML so the old device is untouched. `config_only: true` (offline) and in-place renames rewrite the YAML with no jobs. A non-retargetable `esphome.name` (from packages / `!include` / an embedded substitution) refuses with `INVALID_ARGS` on every path. Both record the pre-rename `esphome.name` (as does a hand-edited one), since the running firmware keeps broadcasting it until the next install, and the device's OTA address-cache args are then published under the new `.local` key using that name's cached addresses. A config-only or in-place rename changes nothing and answers `PRECONDITION_FAILED` when the file changed while the rewrite was being validated, `NOT_FOUND` when it was deleted meanwhile, and the usual name-exists `INVALID_ARGS` when another config took the new filename meanwhile. | | `devices/delete` | `{configuration}` | — | Delete device and associated files. A missing YAML surfaces `not_found` (same as `devices/archive`), not `internal_error`. | | `devices/delete_bulk` | `{configurations: string[]}` | `[{configuration, success, error?}]` | Delete multiple devices | | `devices/archive` | `{configuration}` | — | Soft-delete: move YAML to `/archive/`, wipe build dir, wipe StorageJSON + device-metadata sidecars. Reversible via `devices/unarchive` (cached IP/version/hash refill from the next mDNS broadcast). | diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index fbf651bdb4..e8143ecf3f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -270,9 +270,12 @@ firmware/install {configuration} → QUEUED → RUNNING → output... → COMPLE broadcasting the pre-rename hostname. That name is recorded as `deployed_name` in the device-metadata store and backs the device's OTA address-cache args, so an install still reaches a device that never - announces its new name (#2730). A chained flash-free rename keeps the first - record, renaming back to it clears it, and it also clears on the next - landed flash or completed rename chain. + announces its new name (#2730); the record is dropped when another config + claims that name, since its broadcast is then somebody else's. A + hand-edited `esphome.name` records the same way, off the scan's name + change. A chained flash-free rename keeps the first record, renaming back + to it clears it, and it also clears on the next landed flash or completed + rename chain. - Plus a **remote build-server pool** — one more consumer (`run_dispatch_loop`) gathered alongside the lane workers. Compiles eligible for a paired server hold here (off the single compile lane) and run concurrently, one per diff --git a/esphome_device_builder/controllers/devices/controller.py b/esphome_device_builder/controllers/devices/controller.py index 2d7b96a4cd..07951cc047 100644 --- a/esphome_device_builder/controllers/devices/controller.py +++ b/esphome_device_builder/controllers/devices/controller.py @@ -429,6 +429,10 @@ def get_address_cache_args(self, configuration: str) -> list[str]: if loaded and "api" not in loaded and "web_server" not in loaded: return [] deployed_name = self._metadata_store.get(configuration).get("deployed_name", "") + if deployed_name and self._scanner.get_by_name(deployed_name): + # Another config reclaimed the name, so whatever answers to it + # is no longer this device. + deployed_name = "" return _build_address_cache_args(device, self._state_monitor, deployed_name) def get_ota_address_cache_args(self, configuration: str, port: str | None) -> list[str]: diff --git a/esphome_device_builder/controllers/devices/metadata.py b/esphome_device_builder/controllers/devices/metadata.py index 897adb48bd..47f254fcde 100644 --- a/esphome_device_builder/controllers/devices/metadata.py +++ b/esphome_device_builder/controllers/devices/metadata.py @@ -218,6 +218,16 @@ async def _migrate_device_metadata( await self._metadata_store.rename(old_configuration, new_configuration) await self._shared_sidecar.rename(old_configuration, new_configuration) + def _stamp_deployed_name(self, configuration: str, *, old_name: str, new_name: str) -> None: + """Record the hostname the firmware still answers to after a flash-free rename. + + A chained rename keeps the existing record; renaming back to it clears. + """ + stamped = self._metadata_store.get(configuration).get("deployed_name") or old_name + self._metadata_store.update( + configuration, deployed_name="" if stamped == new_name else stamped + ) + async def _clear_volatile_device_metadata(self, configuration: str) -> None: """Clear archive-volatile fields in both stores (keeps identity). diff --git a/esphome_device_builder/controllers/devices/mutations_simple.py b/esphome_device_builder/controllers/devices/mutations_simple.py index 3003935eac..88796e48dc 100644 --- a/esphome_device_builder/controllers/devices/mutations_simple.py +++ b/esphome_device_builder/controllers/devices/mutations_simple.py @@ -340,22 +340,12 @@ def _land() -> None: await locks.enter_async_context(controller._yaml_write_lock(name)) await run_in_executor(_land) # Before the migrate, so its immediate flush is the only store write. - _stamp_deployed_name(controller, configuration, old_name=old_name, new_name=new_name) + controller._stamp_deployed_name(configuration, old_name=old_name, new_name=new_name) await migrate_metadata(controller, configuration, new_filename) await rescan_renamed(controller, new_filename) return {"configuration": new_filename, "job": None} -def _stamp_deployed_name( - controller: DevicesController, configuration: str, *, old_name: str, new_name: str -) -> None: - """Record the hostname the firmware still answers to after a flash-free rename.""" - stamped = controller._metadata_store.get(configuration).get("deployed_name") or old_name - controller._metadata_store.update( - configuration, deployed_name="" if stamped == new_name else stamped - ) - - def _migrate_storage_json(old_configuration: str, new_filename: str, new_name: str) -> None: """Move the StorageJSON sidecar to the new filename, retargeting the name. diff --git a/esphome_device_builder/controllers/devices/scan_change.py b/esphome_device_builder/controllers/devices/scan_change.py index a63acb136c..f07da0b14a 100644 --- a/esphome_device_builder/controllers/devices/scan_change.py +++ b/esphome_device_builder/controllers/devices/scan_change.py @@ -159,6 +159,11 @@ def _reconcile_rename( or previous.name == device.name ): return + # A hand-edited ``esphome.name`` moves the YAML ahead of the firmware + # exactly like a config-only rename does. + controller._stamp_deployed_name( + device.configuration, old_name=previous.name, new_name=device.name + ) # The ADDED retraction keys on the name at add time; the freed old # name may have a suppressed announcement worth resurfacing. controller._on_importable_removed(device.name) diff --git a/tests/controllers/devices/test_branches_coverage.py b/tests/controllers/devices/test_branches_coverage.py index 8c92a05401..203f9a992a 100644 --- a/tests/controllers/devices/test_branches_coverage.py +++ b/tests/controllers/devices/test_branches_coverage.py @@ -1614,7 +1614,7 @@ def test_on_scan_change_added_without_importable_row_is_silent( assert captured == [] -def test_on_scan_change_reloaded_name_change_prunes_importable_row( +async def test_on_scan_change_reloaded_name_change_prunes_importable_row( tmp_path: Path, make_controller: MakeControllerFactory, capture_devices_events: CaptureDevicesEventsFactory, @@ -1631,7 +1631,7 @@ def test_on_scan_change_reloaded_name_change_prunes_importable_row( assert ("revisit_importable", "kitchen-yaml") in controller._state_monitor.calls -def test_on_scan_change_updated_name_change_prunes_importable_row( +async def test_on_scan_change_updated_name_change_prunes_importable_row( tmp_path: Path, make_controller: MakeControllerFactory, capture_devices_events: CaptureDevicesEventsFactory, @@ -1648,6 +1648,29 @@ def test_on_scan_change_updated_name_change_prunes_importable_row( assert ("revisit_importable", "old-kitchen") in controller._state_monitor.calls +async def test_on_scan_change_name_change_records_the_deployed_name( + tmp_path: Path, make_controller: MakeControllerFactory +) -> None: + """A hand-edited ``esphome.name`` strands the firmware like a config-only rename.""" + controller = make_controller(tmp_path, with_state_monitor=True) + + controller._on_scan_change(ScanChange.UPDATED, _device("livingroom"), _device("kitchen")) + + configuration = _device("livingroom").configuration + assert controller._metadata_store.get(configuration)["deployed_name"] == "kitchen" + + +async def test_on_scan_change_same_name_records_nothing( + tmp_path: Path, make_controller: MakeControllerFactory +) -> None: + """An ordinary edit leaves no record; the firmware still matches the YAML.""" + controller = make_controller(tmp_path, with_state_monitor=True) + + controller._on_scan_change(ScanChange.UPDATED, _device("kitchen"), _device("kitchen")) + + assert controller._metadata_store.get(_device("kitchen").configuration) == {} + + def test_on_scan_change_reloaded_same_name_skips_importable_prune( tmp_path: Path, make_controller: MakeControllerFactory, @@ -1665,7 +1688,7 @@ def test_on_scan_change_reloaded_same_name_skips_importable_prune( assert ("revisit_importable", "kitchen") not in controller._state_monitor.calls -def test_on_scan_change_rename_migrates_monitor_state( +async def test_on_scan_change_rename_migrates_monitor_state( tmp_path: Path, make_controller: MakeControllerFactory ) -> None: """A rename probes the corrected name and forgets the freed name's monitor state.""" @@ -1679,7 +1702,7 @@ def test_on_scan_change_rename_migrates_monitor_state( assert "old-kitchen" not in controller._reachability._ping_last_seen -def test_on_scan_change_rename_keeps_state_for_surviving_sibling( +async def test_on_scan_change_rename_keeps_state_for_surviving_sibling( tmp_path: Path, make_controller: MakeControllerFactory ) -> None: """The freed name's monitor state survives while a sibling YAML still owns it.""" diff --git a/tests/controllers/firmware/test_address_cache.py b/tests/controllers/firmware/test_address_cache.py index 44bf8d9a0e..09c4d4a376 100644 --- a/tests/controllers/firmware/test_address_cache.py +++ b/tests/controllers/firmware/test_address_cache.py @@ -177,6 +177,13 @@ def test_deployed_name_miss_falls_back_to_device_ip() -> None: assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.99"] +def test_deployed_name_cache_beats_the_persisted_ip() -> None: + """A live cache entry for the old name is fresher than the persisted IP.""" + monitor = RecordingStateMonitor(cached_addresses={"asistente.local": ["192.168.1.50"]}) + args = _build_address_cache_args(_device(ip="192.168.1.99"), monitor, "asistente") + assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.50"] + + def test_deployed_name_ignored_for_non_local_address() -> None: """A renamed ``.local`` name is irrelevant to a DNS-resolved address.""" monitor = RecordingStateMonitor(cached_addresses={"asistente.local": ["192.168.1.50"]}) @@ -190,7 +197,9 @@ def test_deployed_name_ignored_for_non_local_address() -> None: # ---------------------------------------------------------------------- -def _devices_controller_with(*devices: Device, deployed_name: str = "") -> Any: +def _devices_controller_with( + *devices: Device, deployed_name: str = "", names_taken: tuple[str, ...] = () +) -> Any: """Build a thin DevicesController shell with a stubbed scanner + monitor. ``get_address_cache_args`` reads the scanner's configuration-keyed @@ -200,7 +209,7 @@ def _devices_controller_with(*devices: Device, deployed_name: str = "") -> Any: of the test surface. """ controller = DevicesController.__new__(DevicesController) - scanner = RecordingScanner() + scanner = RecordingScanner(devices_by_name={name: [object()] for name in names_taken}) scanner.devices = list(devices) controller._scanner = scanner controller._state_monitor = _monitor(["192.168.1.50"]) @@ -290,6 +299,22 @@ def test_get_address_cache_args_filename_differs_from_device_name() -> None: ] +def test_get_address_cache_args_drops_a_reclaimed_deployed_name() -> None: + """A config that took the old name owns its broadcast; fall back to the IP.""" + controller = _devices_controller_with( + _device(loaded_integrations=["api"], ip="192.168.1.99"), + deployed_name="asistente", + names_taken=("asistente",), + ) + controller._state_monitor = RecordingStateMonitor( + cached_addresses={"asistente.local": ["10.0.0.1"]} + ) + + args = controller.get_address_cache_args("kitchen.yaml") + + assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.99"] + + def test_get_ota_address_cache_args_returns_cache_for_ota_port() -> None: """Strict ``port == "OTA"`` → delegate to ``get_address_cache_args``.""" controller = _devices_controller_with(_device()) From a3b93b0574bf20b33f395e1e6ba94955c7664585 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 05:07:55 +0100 Subject: [PATCH 03/15] Gate the scan-change record on a prior compile and self-heal on mDNS The cold-start shallow to deep refine renames a package-supplied name off its filename-stem placeholder, which no firmware answered to, so only stamp for a device that was compiled at least once. Clear the record when the device is heard over mDNS under its own name, so a flash from outside the dashboard heals it. --- docs/ARCHITECTURE.md | 8 ++++-- .../controllers/devices/scan_change.py | 12 ++++---- .../controllers/devices/state_callbacks.py | 4 +++ .../devices/test_branches_coverage.py | 28 ++++++++++++++++--- tests/test_device_state_event.py | 22 +++++++++++++++ 5 files changed, 62 insertions(+), 12 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e8143ecf3f..9523951d6c 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -273,9 +273,11 @@ firmware/install {configuration} → QUEUED → RUNNING → output... → COMPLE announces its new name (#2730); the record is dropped when another config claims that name, since its broadcast is then somebody else's. A hand-edited `esphome.name` records the same way, off the scan's name - change. A chained flash-free rename keeps the first record, renaming back - to it clears it, and it also clears on the next landed flash or completed - rename chain. + change, for a device that was compiled at least once. A chained flash-free + rename keeps the first record; it clears on a rename back to it, on the + next landed flash or completed rename chain, and on an mDNS ONLINE under + the device's own name (the self-heal for a flash from outside the + dashboard). - Plus a **remote build-server pool** — one more consumer (`run_dispatch_loop`) gathered alongside the lane workers. Compiles eligible for a paired server hold here (off the single compile lane) and run concurrently, one per diff --git a/esphome_device_builder/controllers/devices/scan_change.py b/esphome_device_builder/controllers/devices/scan_change.py index f07da0b14a..83e401da26 100644 --- a/esphome_device_builder/controllers/devices/scan_change.py +++ b/esphome_device_builder/controllers/devices/scan_change.py @@ -159,11 +159,13 @@ def _reconcile_rename( or previous.name == device.name ): return - # A hand-edited ``esphome.name`` moves the YAML ahead of the firmware - # exactly like a config-only rename does. - controller._stamp_deployed_name( - device.configuration, old_name=previous.name, new_name=device.name - ) + # A hand-edited name strands the firmware like a config-only rename. + # Never-compiled devices are skipped: the cold-start shallow→deep + # refine lands here too, off a filename-stem placeholder. + if device.loaded_integrations: + controller._stamp_deployed_name( + device.configuration, old_name=previous.name, new_name=device.name + ) # The ADDED retraction keys on the name at add time; the freed old # name may have a suppressed announcement worth resurfacing. controller._on_importable_removed(device.name) diff --git a/esphome_device_builder/controllers/devices/state_callbacks.py b/esphome_device_builder/controllers/devices/state_callbacks.py index b0d9f54344..62323efff8 100644 --- a/esphome_device_builder/controllers/devices/state_callbacks.py +++ b/esphome_device_builder/controllers/devices/state_callbacks.py @@ -55,7 +55,11 @@ def on_state_change( controller: DevicesController, name: str, state: DeviceState, source: str ) -> None: """Forward state monitor updates onto the event bus.""" + # An announce under the device's own name proves the firmware carries it. + heard_own_name = state is DeviceState.ONLINE and source == ReachabilitySource.MDNS for device in controller._devices_by_name(name): + if heard_own_name: + controller._metadata_store.update(device.configuration, deployed_name="") old_state = device.runtime_state.state device.runtime_state.state = state _LOGGER.info( diff --git a/tests/controllers/devices/test_branches_coverage.py b/tests/controllers/devices/test_branches_coverage.py index 203f9a992a..9c9fbe30b3 100644 --- a/tests/controllers/devices/test_branches_coverage.py +++ b/tests/controllers/devices/test_branches_coverage.py @@ -65,12 +65,19 @@ ) -def _device(name: str, *, ip: str = "", ip_addresses: list[str] | None = None) -> Device: +def _device( + name: str, + *, + ip: str = "", + ip_addresses: list[str] | None = None, + loaded_integrations: list[str] | None = None, +) -> Device: return make_device( name=name, state=DeviceState.ONLINE, ip=ip, ip_addresses=list(ip_addresses) if ip_addresses else [], + loaded_integrations=loaded_integrations or [], ) @@ -1654,10 +1661,23 @@ async def test_on_scan_change_name_change_records_the_deployed_name( """A hand-edited ``esphome.name`` strands the firmware like a config-only rename.""" controller = make_controller(tmp_path, with_state_monitor=True) - controller._on_scan_change(ScanChange.UPDATED, _device("livingroom"), _device("kitchen")) + compiled = _device("livingroom", loaded_integrations=["api"]) + + controller._on_scan_change(ScanChange.UPDATED, compiled, _device("kitchen")) + + assert controller._metadata_store.get(compiled.configuration)["deployed_name"] == "kitchen" + + +async def test_on_scan_change_never_compiled_name_change_records_nothing( + tmp_path: Path, make_controller: MakeControllerFactory +) -> None: + """The cold-start refine renames off a placeholder; no firmware answered to it.""" + controller = make_controller(tmp_path, with_state_monitor=True) + refined = _device("livingroom") + + controller._on_scan_change(ScanChange.RELOADED, refined, _device("livingroom-yaml")) - configuration = _device("livingroom").configuration - assert controller._metadata_store.get(configuration)["deployed_name"] == "kitchen" + assert controller._metadata_store.get(refined.configuration) == {} async def test_on_scan_change_same_name_records_nothing( diff --git a/tests/test_device_state_event.py b/tests/test_device_state_event.py index a135c6f4da..6a985a7cd5 100644 --- a/tests/test_device_state_event.py +++ b/tests/test_device_state_event.py @@ -58,3 +58,25 @@ def test_state_change_unknown_device_does_not_fire() -> None: ctrl._on_state_change("ghost", DeviceState.ONLINE, "mdns") assert captured == [] + + +async def test_mdns_online_under_its_own_name_clears_the_deployed_name() -> None: + """An announce under the YAML's name proves the firmware carries it (#2730).""" + device = make_device(address="") + ctrl, _ = make_devices_controller_with_bus([device]) + ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + + ctrl._on_state_change("kitchen", DeviceState.ONLINE, "mdns") + + assert "deployed_name" not in ctrl._metadata_store.get("kitchen.yaml") + + +async def test_ping_online_keeps_the_deployed_name() -> None: + """Ping reaches the device through the record itself; it proves no name.""" + device = make_device(address="") + ctrl, _ = make_devices_controller_with_bus([device]) + ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + + ctrl._on_state_change("kitchen", DeviceState.ONLINE, "ping") + + assert ctrl._metadata_store.get("kitchen.yaml")["deployed_name"] == "asistente" From 5ef39951fe2627ecdc81452590a9d8157efbce63 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 08:56:54 +0100 Subject: [PATCH 04/15] Say what the scan-change gate actually checks --only-generate populates loaded_integrations without flashing, so the gate means the device has build output, not that it was ever flashed. --- docs/ARCHITECTURE.md | 2 +- esphome_device_builder/controllers/devices/scan_change.py | 4 ++-- tests/controllers/devices/test_branches_coverage.py | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9523951d6c..5ef46a098c 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -273,7 +273,7 @@ firmware/install {configuration} → QUEUED → RUNNING → output... → COMPLE announces its new name (#2730); the record is dropped when another config claims that name, since its broadcast is then somebody else's. A hand-edited `esphome.name` records the same way, off the scan's name - change, for a device that was compiled at least once. A chained flash-free + change, for a device with build output. A chained flash-free rename keeps the first record; it clears on a rename back to it, on the next landed flash or completed rename chain, and on an mDNS ONLINE under the device's own name (the self-heal for a flash from outside the diff --git a/esphome_device_builder/controllers/devices/scan_change.py b/esphome_device_builder/controllers/devices/scan_change.py index 83e401da26..925328f973 100644 --- a/esphome_device_builder/controllers/devices/scan_change.py +++ b/esphome_device_builder/controllers/devices/scan_change.py @@ -160,8 +160,8 @@ def _reconcile_rename( ): return # A hand-edited name strands the firmware like a config-only rename. - # Never-compiled devices are skipped: the cold-start shallow→deep - # refine lands here too, off a filename-stem placeholder. + # Gated on build output: the cold-start shallow→deep refine lands + # here too, renaming off a filename-stem placeholder. if device.loaded_integrations: controller._stamp_deployed_name( device.configuration, old_name=previous.name, new_name=device.name diff --git a/tests/controllers/devices/test_branches_coverage.py b/tests/controllers/devices/test_branches_coverage.py index 9c9fbe30b3..efd3dc2c16 100644 --- a/tests/controllers/devices/test_branches_coverage.py +++ b/tests/controllers/devices/test_branches_coverage.py @@ -1668,10 +1668,10 @@ async def test_on_scan_change_name_change_records_the_deployed_name( assert controller._metadata_store.get(compiled.configuration)["deployed_name"] == "kitchen" -async def test_on_scan_change_never_compiled_name_change_records_nothing( +async def test_on_scan_change_unbuilt_name_change_records_nothing( tmp_path: Path, make_controller: MakeControllerFactory ) -> None: - """The cold-start refine renames off a placeholder; no firmware answered to it.""" + """The cold-start refine renames off a placeholder; no build output backs it.""" controller = make_controller(tmp_path, with_state_monitor=True) refined = _device("livingroom") From a1c78a91244ca165452e83955c450ee25a63dd28 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 10:48:41 +0100 Subject: [PATCH 05/15] Let the rename's record outlive its own rescan An in-place rescan re-enters the scan-change stamp with the pre-rename name, resurrecting a record the rename just cleared. Re-assert what the rename resolved once the rescan is done; a no-op on every other path. Also say the reclaimed-name record is ignored, not dropped: the read skips it while another config owns the name, the entry stays on disk. --- docs/ARCHITECTURE.md | 4 +-- .../controllers/devices/metadata.py | 11 ++++---- .../controllers/devices/mutations_simple.py | 9 ++++-- .../devices/test_rename_config_only.py | 28 +++++++++++++++++++ 4 files changed, 43 insertions(+), 9 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 5ef46a098c..e155206339 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -270,8 +270,8 @@ firmware/install {configuration} → QUEUED → RUNNING → output... → COMPLE broadcasting the pre-rename hostname. That name is recorded as `deployed_name` in the device-metadata store and backs the device's OTA address-cache args, so an install still reaches a device that never - announces its new name (#2730); the record is dropped when another config - claims that name, since its broadcast is then somebody else's. A + announces its new name (#2730); the record is ignored while another config + owns that name, since its broadcast is then somebody else's. A hand-edited `esphome.name` records the same way, off the scan's name change, for a device with build output. A chained flash-free rename keeps the first record; it clears on a rename back to it, on the diff --git a/esphome_device_builder/controllers/devices/metadata.py b/esphome_device_builder/controllers/devices/metadata.py index 47f254fcde..584593f76f 100644 --- a/esphome_device_builder/controllers/devices/metadata.py +++ b/esphome_device_builder/controllers/devices/metadata.py @@ -218,15 +218,16 @@ async def _migrate_device_metadata( await self._metadata_store.rename(old_configuration, new_configuration) await self._shared_sidecar.rename(old_configuration, new_configuration) - def _stamp_deployed_name(self, configuration: str, *, old_name: str, new_name: str) -> None: + def _stamp_deployed_name(self, configuration: str, *, old_name: str, new_name: str) -> str: """Record the hostname the firmware still answers to after a flash-free rename. - A chained rename keeps the existing record; renaming back to it clears. + A chained rename keeps the existing record; renaming back to it + clears. Returns the recorded name (``""`` when cleared). """ stamped = self._metadata_store.get(configuration).get("deployed_name") or old_name - self._metadata_store.update( - configuration, deployed_name="" if stamped == new_name else stamped - ) + deployed = "" if stamped == new_name else stamped + self._metadata_store.update(configuration, deployed_name=deployed) + return deployed async def _clear_volatile_device_metadata(self, configuration: str) -> None: """Clear archive-volatile fields in both stores (keeps identity). diff --git a/esphome_device_builder/controllers/devices/mutations_simple.py b/esphome_device_builder/controllers/devices/mutations_simple.py index 88796e48dc..507fcd51d8 100644 --- a/esphome_device_builder/controllers/devices/mutations_simple.py +++ b/esphome_device_builder/controllers/devices/mutations_simple.py @@ -339,10 +339,15 @@ def _land() -> None: for name in sorted({os.path.normpath(n) for n in (configuration, new_filename)}): await locks.enter_async_context(controller._yaml_write_lock(name)) await run_in_executor(_land) - # Before the migrate, so its immediate flush is the only store write. - controller._stamp_deployed_name(configuration, old_name=old_name, new_name=new_name) + # Before the migrate, so its immediate flush carries the record. + deployed_name = controller._stamp_deployed_name( + configuration, old_name=old_name, new_name=new_name + ) await migrate_metadata(controller, configuration, new_filename) await rescan_renamed(controller, new_filename) + # An in-place rescan re-enters the scan-change stamp with the + # pre-rename name; this rename knows better. A no-op otherwise. + controller._metadata_store.update(new_filename, deployed_name=deployed_name) return {"configuration": new_filename, "job": None} diff --git a/tests/controllers/devices/test_rename_config_only.py b/tests/controllers/devices/test_rename_config_only.py index 6c82045368..541dcfd413 100644 --- a/tests/controllers/devices/test_rename_config_only.py +++ b/tests/controllers/devices/test_rename_config_only.py @@ -9,11 +9,13 @@ import pytest from esphome.storage_json import StorageJSON +from esphome_device_builder.controllers._device_scanner import ScanChange from esphome_device_builder.controllers.devices import mutations_simple from esphome_device_builder.helpers.api import CommandError from esphome_device_builder.helpers.yaml import read_yaml_scalar from esphome_device_builder.models import ErrorCode from tests._storage_fixtures import write_storage_json +from tests.conftest import make_device from .conftest import MakeControllerFactory, wifi_ap_block @@ -493,3 +495,29 @@ async def test_config_only_rename_records_the_deployed_name( assert current == expected_filename assert controller._metadata_store.get(current).get("deployed_name") == expected_deployed + + +async def test_in_place_rename_back_survives_the_rescan_stamp( + tmp_path: Path, make_controller: MakeControllerFactory +) -> None: + """The rescan re-enters the scan-change stamp with the pre-rename name (#2730).""" + controller = make_controller(tmp_path, with_state_monitor=True) + (tmp_path / "kitchen.yaml").write_text( + _YAML.replace("name: kitchen", "name: livingroom"), encoding="utf-8" + ) + # The state a hand-edit leaves: the firmware still answers to kitchen. + controller._metadata_store.update("kitchen.yaml", deployed_name="kitchen", delay=0.0) + + async def _rescan_fires_scan_change(_controller: object, configuration: str) -> None: + controller._on_scan_change( + ScanChange.RELOADED, + make_device(configuration=configuration, name="kitchen", loaded_integrations=["api"]), + make_device(configuration=configuration, name="livingroom"), + ) + + with patch.object(mutations_simple, "rescan_renamed", _rescan_fires_scan_change): + await controller.rename_device( + configuration="kitchen.yaml", new_name="kitchen", config_only=True + ) + + assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") From a14b32d369e718f5427ac68d8d428a6158dc0926 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 22:37:51 +0100 Subject: [PATCH 06/15] Skip the post-rescan re-assert when the entry moved away The re-assert runs outside the write locks, so a concurrent rename can migrate the entry between the rescan and it, and a truthy record would re-create an orphan row nothing reads. --- .../controllers/devices/mutations_simple.py | 7 ++++--- .../devices/test_rename_config_only.py | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/esphome_device_builder/controllers/devices/mutations_simple.py b/esphome_device_builder/controllers/devices/mutations_simple.py index 507fcd51d8..70f6c2348a 100644 --- a/esphome_device_builder/controllers/devices/mutations_simple.py +++ b/esphome_device_builder/controllers/devices/mutations_simple.py @@ -345,9 +345,10 @@ def _land() -> None: ) await migrate_metadata(controller, configuration, new_filename) await rescan_renamed(controller, new_filename) - # An in-place rescan re-enters the scan-change stamp with the - # pre-rename name; this rename knows better. A no-op otherwise. - controller._metadata_store.update(new_filename, deployed_name=deployed_name) + # An in-place rescan re-stamps the pre-rename name; this rename knows + # better. Skipped when a concurrent rename moved the entry away. + if controller._metadata_store.get(new_filename): + controller._metadata_store.update(new_filename, deployed_name=deployed_name) return {"configuration": new_filename, "job": None} diff --git a/tests/controllers/devices/test_rename_config_only.py b/tests/controllers/devices/test_rename_config_only.py index 541dcfd413..76247a7517 100644 --- a/tests/controllers/devices/test_rename_config_only.py +++ b/tests/controllers/devices/test_rename_config_only.py @@ -521,3 +521,21 @@ async def _rescan_fires_scan_change(_controller: object, configuration: str) -> ) assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") + + +async def test_rename_leaves_no_orphan_when_the_entry_moved_away( + tmp_path: Path, make_controller: MakeControllerFactory +) -> None: + """A concurrent rename migrating the entry must not get it re-created (#2730).""" + controller = make_controller(tmp_path) + (tmp_path / "kitchen.yaml").write_text(_YAML, encoding="utf-8") + + async def _rescan_steals_the_entry(_controller: object, configuration: str) -> None: + await controller._metadata_store.remove(configuration) + + with patch.object(mutations_simple, "rescan_renamed", _rescan_steals_the_entry): + await controller.rename_device( + configuration="kitchen.yaml", new_name="livingroom", config_only=True + ) + + assert controller._metadata_store.get("livingroom.yaml") == {} From f6c7d9c2765623595f72aea2a63ff539f7c0f35c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 22:57:28 +0100 Subject: [PATCH 07/15] Tighten the deployed_name clear paths A bootloader-only upload replaces no app, so it no longer clears the record. The mDNS self-heal moves onto the source change: apply() skips the state callback when the device is already ONLINE via ping, which is exactly the case the record keeps alive. The post-rescan re-assert narrows to the clear, the only value a rescan can pollute, so it can't overwrite a fresher mDNS or concurrent-rename value. --- docs/ARCHITECTURE.md | 8 +- .../controllers/devices/firmware_sync.py | 7 +- .../controllers/devices/mutations_simple.py | 10 ++- .../controllers/devices/state_callbacks.py | 9 ++- tests/controllers/firmware/test_refresh.py | 73 ++++++++++++------- tests/test_device_state_event.py | 20 ++++- 6 files changed, 83 insertions(+), 44 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e155206339..9d4dc2b000 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -275,9 +275,11 @@ firmware/install {configuration} → QUEUED → RUNNING → output... → COMPLE hand-edited `esphome.name` records the same way, off the scan's name change, for a device with build output. A chained flash-free rename keeps the first record; it clears on a rename back to it, on the - next landed flash or completed rename chain, and on an mDNS ONLINE under - the device's own name (the self-heal for a flash from outside the - dashboard). + next app flash or completed rename chain (a `--bootloader` upload replaces + no app, so it keeps the record), and when mDNS takes ownership of the + device's own name (the self-heal for a flash from outside the dashboard, + keyed on ownership rather than the state flip so a ping-online device is + covered). - Plus a **remote build-server pool** — one more consumer (`run_dispatch_loop`) gathered alongside the lane workers. Compiles eligible for a paired server hold here (off the single compile lane) and run concurrently, one per diff --git a/esphome_device_builder/controllers/devices/firmware_sync.py b/esphome_device_builder/controllers/devices/firmware_sync.py index ea2c6f579e..0b696eacc2 100644 --- a/esphome_device_builder/controllers/devices/firmware_sync.py +++ b/esphome_device_builder/controllers/devices/firmware_sync.py @@ -90,6 +90,10 @@ def on_job_completed(controller: DevicesController, event: Event[JobLifecycleDat return recompute_hash = job_type in COMPILING_JOB_TYPES flashed = job_type in (JobType.UPLOAD, JobType.INSTALL) + if flashed and not job.flash_bootloader: + # The app image now carries the YAML's own name; a bootloader-only + # upload replaces no app, so its record stands. + controller._metadata_store.update(configuration, deployed_name="") # Routed through the controller's bound delegate so tests # that monkeypatch ``_refresh_after_firmware_job`` on the # instance still intercept. @@ -122,9 +126,6 @@ async def refresh_after_job( await controller._persist_expected_config_hash(configuration) await controller._scanner.reload(configuration) if flashed: - # The landed image carries the YAML's own name, so a pre-rename - # hostname recorded for it is spent. - controller._metadata_store.update(configuration, deployed_name="") await controller._sync_deployed_state_after_flash(configuration) controller._schedule_version_reprobe(configuration) # A real compile moves the build-size cache's freshness diff --git a/esphome_device_builder/controllers/devices/mutations_simple.py b/esphome_device_builder/controllers/devices/mutations_simple.py index 70f6c2348a..a65999ac5f 100644 --- a/esphome_device_builder/controllers/devices/mutations_simple.py +++ b/esphome_device_builder/controllers/devices/mutations_simple.py @@ -345,10 +345,12 @@ def _land() -> None: ) await migrate_metadata(controller, configuration, new_filename) await rescan_renamed(controller, new_filename) - # An in-place rescan re-stamps the pre-rename name; this rename knows - # better. Skipped when a concurrent rename moved the entry away. - if controller._metadata_store.get(new_filename): - controller._metadata_store.update(new_filename, deployed_name=deployed_name) + # An in-place rescan re-stamps the pre-rename name over a record this + # rename just cleared. Only the clear is re-asserted: the stamp keeps + # an existing record, so a set is never the value that got polluted, + # and re-clearing never overwrites a fresher mDNS or rename value. + if not deployed_name: + controller._metadata_store.update(new_filename, deployed_name="") return {"configuration": new_filename, "job": None} diff --git a/esphome_device_builder/controllers/devices/state_callbacks.py b/esphome_device_builder/controllers/devices/state_callbacks.py index 62323efff8..e804b4d188 100644 --- a/esphome_device_builder/controllers/devices/state_callbacks.py +++ b/esphome_device_builder/controllers/devices/state_callbacks.py @@ -55,11 +55,7 @@ def on_state_change( controller: DevicesController, name: str, state: DeviceState, source: str ) -> None: """Forward state monitor updates onto the event bus.""" - # An announce under the device's own name proves the firmware carries it. - heard_own_name = state is DeviceState.ONLINE and source == ReachabilitySource.MDNS for device in controller._devices_by_name(name): - if heard_own_name: - controller._metadata_store.update(device.configuration, deployed_name="") old_state = device.runtime_state.state device.runtime_state.state = state _LOGGER.info( @@ -88,6 +84,11 @@ def on_source_change(controller: DevicesController, name: str, source: Reachabil for device in controller._devices_by_name(name): if device.runtime_state.active_source == source: continue + if source is ReachabilitySource.MDNS: + # An announce under the device's own name proves the firmware + # carries it. Ownership, not the state flip: a ping-online + # device stays ONLINE, so only the source moves. + controller._metadata_store.update(device.configuration, deployed_name="") device.runtime_state.active_source = source controller._fire_device_updated(device) diff --git a/tests/controllers/firmware/test_refresh.py b/tests/controllers/firmware/test_refresh.py index c6e9ca9d28..41e90c0e32 100644 --- a/tests/controllers/firmware/test_refresh.py +++ b/tests/controllers/firmware/test_refresh.py @@ -115,7 +115,7 @@ async def test_reload_unknown_filename_is_noop(tmp_path: Path) -> None: # ---------------------------------------------------------------------- -def _make_controller() -> tuple[Any, list[tuple[str, bool, bool]]]: +def _make_controller(tmp_path: Path) -> tuple[Any, list[tuple[str, bool, bool]]]: """Build a partially-initialised controller and a capture list. ``_refresh_after_firmware_job`` is patched with a sync stub that @@ -145,6 +145,12 @@ async def _noop() -> None: # without needing the full worker lifecycle. controller._build_size = MagicMock() controller._refresh_after_firmware_job = _capturing_refresh # type: ignore[method-assign] + controller._shutdown_callbacks = [] + controller._metadata_store = DeviceMetadataStore( + config_dir=tmp_path, + data_dir=tmp_path, + shutdown_register=controller._shutdown_callbacks.append, + ) return controller, captured @@ -157,9 +163,9 @@ def _job(job_type: JobType, status: JobStatus, configuration: str = "kitchen.yam ) -def test_completed_install_recomputes_hash_and_reloads() -> None: +def test_completed_install_recomputes_hash_and_reloads(tmp_path: Path) -> None: """A successful INSTALL recompiles + flashes → hash is fresh, persist it.""" - controller, captured = _make_controller() + controller, captured = _make_controller(tmp_path) job = _job(JobType.INSTALL, JobStatus.COMPLETED) controller._on_firmware_job_completed(Event(EventType.JOB_COMPLETED, {"job": job})) @@ -170,9 +176,33 @@ def test_completed_install_recomputes_hash_and_reloads() -> None: assert captured == [("kitchen.yaml", True, True)] -def test_completed_compile_recomputes_hash_and_reloads() -> None: +async def test_completed_app_upload_clears_the_deployed_name(tmp_path: Path) -> None: + """The app image now carries the YAML's own name (#2730).""" + controller, _ = _make_controller(tmp_path) + controller._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + + controller._on_firmware_job_completed( + Event(EventType.JOB_COMPLETED, {"job": _job(JobType.UPLOAD, JobStatus.COMPLETED)}) + ) + + assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") + + +async def test_completed_bootloader_upload_keeps_the_deployed_name(tmp_path: Path) -> None: + """``--bootloader`` replaces no app, so the recorded hostname still stands.""" + controller, _ = _make_controller(tmp_path) + controller._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + job = _job(JobType.UPLOAD, JobStatus.COMPLETED) + job.flash_bootloader = True + + controller._on_firmware_job_completed(Event(EventType.JOB_COMPLETED, {"job": job})) + + assert controller._metadata_store.get("kitchen.yaml")["deployed_name"] == "asistente" + + +def test_completed_compile_recomputes_hash_and_reloads(tmp_path: Path) -> None: """COMPILE produces a new binary tied to a (potentially) new YAML hash.""" - controller, captured = _make_controller() + controller, captured = _make_controller(tmp_path) job = _job(JobType.COMPILE, JobStatus.COMPLETED) controller._on_firmware_job_completed(Event(EventType.JOB_COMPLETED, {"job": job})) @@ -183,9 +213,9 @@ def test_completed_compile_recomputes_hash_and_reloads() -> None: assert captured == [("kitchen.yaml", True, False)] -def test_completed_upload_reloads_without_recomputing_hash() -> None: +def test_completed_upload_reloads_without_recomputing_hash(tmp_path: Path) -> None: """UPLOAD doesn't recompile — the persisted hash from prior compile still applies.""" - controller, captured = _make_controller() + controller, captured = _make_controller(tmp_path) job = _job(JobType.UPLOAD, JobStatus.COMPLETED) controller._on_firmware_job_completed(Event(EventType.JOB_COMPLETED, {"job": job})) @@ -196,9 +226,9 @@ def test_completed_upload_reloads_without_recomputing_hash() -> None: assert captured == [("kitchen.yaml", False, True)] -def test_failed_job_does_not_schedule_refresh() -> None: +def test_failed_job_does_not_schedule_refresh(tmp_path: Path) -> None: """FAILED jobs leave the device's pending state alone.""" - controller, captured = _make_controller() + controller, captured = _make_controller(tmp_path) job = _job(JobType.INSTALL, JobStatus.FAILED) controller._on_firmware_job_completed(Event(EventType.JOB_COMPLETED, {"job": job})) @@ -206,7 +236,7 @@ def test_failed_job_does_not_schedule_refresh() -> None: assert captured == [] -def test_clean_job_skips_full_refresh_but_pokes_build_size() -> None: +def test_clean_job_skips_full_refresh_but_pokes_build_size(tmp_path: Path) -> None: """CLEAN skips the hash / flash bookkeeping path but pokes the build-size cache. The build tree has just been wiped, so the cached @@ -217,7 +247,7 @@ def test_clean_job_skips_full_refresh_but_pokes_build_size() -> None: cache. ``_refresh_after_firmware_job`` (hash recompute, optimistic flash sync) doesn't apply to CLEAN. """ - controller, captured = _make_controller() + controller, captured = _make_controller(tmp_path) job = _job(JobType.CLEAN, JobStatus.COMPLETED) controller._on_firmware_job_completed(Event(EventType.JOB_COMPLETED, {"job": job})) @@ -226,9 +256,9 @@ def test_clean_job_skips_full_refresh_but_pokes_build_size() -> None: controller._build_size.request.assert_called_once_with("kitchen.yaml") -def test_reset_build_env_does_not_schedule_refresh() -> None: +def test_reset_build_env_does_not_schedule_refresh(tmp_path: Path) -> None: """RESET_BUILD_ENV has no per-device configuration to refresh.""" - controller, captured = _make_controller() + controller, captured = _make_controller(tmp_path) job = _job(JobType.RESET_BUILD_ENV, JobStatus.COMPLETED, configuration="") controller._on_firmware_job_completed(Event(EventType.JOB_COMPLETED, {"job": job})) @@ -236,9 +266,9 @@ def test_reset_build_env_does_not_schedule_refresh() -> None: assert captured == [] -def test_receiver_side_remote_build_job_skips_refresh() -> None: +def test_receiver_side_remote_build_job_skips_refresh(tmp_path: Path) -> None: """Remote-build configurations skip the refresh and build-size hooks.""" - controller, captured = _make_controller() + controller, captured = _make_controller(tmp_path) job = _job( JobType.INSTALL, JobStatus.COMPLETED, @@ -251,7 +281,7 @@ def test_receiver_side_remote_build_job_skips_refresh() -> None: controller._build_size.request.assert_not_called() -def test_unhandled_job_type_with_configuration_falls_through_silently() -> None: +def test_unhandled_job_type_with_configuration_falls_through_silently(tmp_path: Path) -> None: """Job types outside CLEAN/COMPILE/UPLOAD/INSTALL/RENAME bail at the type check. Belt-and-braces test for the post-CLEAN dispatch table — a @@ -261,7 +291,7 @@ def test_unhandled_job_type_with_configuration_falls_through_silently() -> None: *after* the empty-configuration short-circuit, leaving the refresh + build-size hooks alone. """ - controller, captured = _make_controller() + controller, captured = _make_controller(tmp_path) job = _job( JobType.RESET_BUILD_ENV, JobStatus.COMPLETED, @@ -377,20 +407,11 @@ async def _fake_compute(path: Path) -> str | None: controller._state_monitor = MagicMock() # The flashed branch arms a post-flash re-probe timer. controller._reprobe_timers = {} - controller._shutdown_callbacks = [] - controller._metadata_store = DeviceMetadataStore( - config_dir=tmp_path, - data_dir=tmp_path, - shutdown_register=controller._shutdown_callbacks.append, - ) - controller._metadata_store.update("kitchen.yaml", deployed_name="old-kitchen", delay=0.0) await controller._refresh_after_firmware_job("kitchen.yaml", recompute_hash=False, flashed=True) assert compute_calls == [] assert controller._scanner.calls == [("reload", "kitchen.yaml")] - # The landed image carries the YAML's own name. - assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") controller._cancel_reprobe_timers() diff --git a/tests/test_device_state_event.py b/tests/test_device_state_event.py index 6a985a7cd5..685ce7d26e 100644 --- a/tests/test_device_state_event.py +++ b/tests/test_device_state_event.py @@ -10,7 +10,7 @@ from __future__ import annotations -from esphome_device_builder.models import DeviceState, EventType +from esphome_device_builder.models import DeviceState, EventType, ReachabilitySource from .conftest import make_device, make_devices_controller_with_bus @@ -60,23 +60,35 @@ def test_state_change_unknown_device_does_not_fire() -> None: assert captured == [] -async def test_mdns_online_under_its_own_name_clears_the_deployed_name() -> None: +async def test_mdns_ownership_clears_the_deployed_name() -> None: """An announce under the YAML's name proves the firmware carries it (#2730).""" device = make_device(address="") ctrl, _ = make_devices_controller_with_bus([device]) ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + ctrl._on_source_change("kitchen", ReachabilitySource.MDNS) + + assert "deployed_name" not in ctrl._metadata_store.get("kitchen.yaml") + + +async def test_mdns_ownership_of_a_ping_online_device_still_clears() -> None: + """``apply`` skips the state callback when the device is already ONLINE.""" + device = make_device(address="", state=DeviceState.ONLINE) + ctrl, _ = make_devices_controller_with_bus([device]) + ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + ctrl._on_state_change("kitchen", DeviceState.ONLINE, "mdns") + ctrl._on_source_change("kitchen", ReachabilitySource.MDNS) assert "deployed_name" not in ctrl._metadata_store.get("kitchen.yaml") -async def test_ping_online_keeps_the_deployed_name() -> None: +async def test_ping_ownership_keeps_the_deployed_name() -> None: """Ping reaches the device through the record itself; it proves no name.""" device = make_device(address="") ctrl, _ = make_devices_controller_with_bus([device]) ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) - ctrl._on_state_change("kitchen", DeviceState.ONLINE, "ping") + ctrl._on_source_change("kitchen", ReachabilitySource.PING) assert ctrl._metadata_store.get("kitchen.yaml")["deployed_name"] == "asistente" From 70a25e74c7f227823fdfab48e14eafcb0d6c0c91 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 23:09:13 +0100 Subject: [PATCH 08/15] Confine the deployed_name key behind accessors Five call sites open-coded the store key and its empty-means-cleared sentinel. Add _clear_deployed_name beside _stamp_deployed_name and a _deployed_name reader that owns the reclaimed-name rule, so the deferred sweep reader inherits it instead of copying the guard. The store gains a non-copying get_field, so the clear on every mDNS ownership change no longer copies an entry to find the key absent. Trim the docstrings and comments the record accumulated, hoist the duplicated metadata-store test scaffolding into tests/conftest.py, and move the lifecycle prose to the live-state section it belongs in. --- docs/API.md | 2 +- docs/ARCHITECTURE.md | 22 +++------- .../controllers/devices/_metadata_store.py | 7 +++ .../controllers/devices/controller.py | 19 +++++--- .../controllers/devices/firmware_sync.py | 12 +++--- .../controllers/devices/helpers.py | 7 +-- .../controllers/devices/metadata.py | 15 ++++--- .../controllers/devices/mutations_simple.py | 7 ++- .../controllers/devices/scan_change.py | 6 +-- .../controllers/devices/state_callbacks.py | 7 ++- tests/conftest.py | 17 +++++--- .../firmware/test_address_cache.py | 43 ++++++++++--------- tests/controllers/firmware/test_refresh.py | 33 +++----------- 13 files changed, 92 insertions(+), 105 deletions(-) diff --git a/docs/API.md b/docs/API.md index a4ceaec496..d5234b122d 100644 --- a/docs/API.md +++ b/docs/API.md @@ -126,7 +126,7 @@ Connections that arrive on the trusted ingress site (HA add-on supervisor proxy) | `devices/update` | `{configuration, friendly_name?, comment?, board_id?}` | `UpdateDeviceResponse` | Update device metadata (sidecar JSON) | | `devices/set_labels` | `{configuration, label_ids: string[]}` | `Device` | Replace this device's label assignments. Pass `[]` to clear. Unknown ids return `INVALID_ARGS`. Fires `device_updated` after the scanner reload when the assignments changed the row. | | `devices/set_labels_bulk` | `{updates: [{configuration, label_ids: string[]}, ...]}` | `[{configuration, success, error?}]` | Replace label assignments across multiple devices. One result per entry preserving input order (duplicates in `updates` produce duplicate rows). Per-entry failures (unknown id, missing device, malformed row) don't block the rest. Rows whose `configuration` can't be extracted (non-dict row, non-string value) surface as `{configuration: "", success: false, error}` so the frontend dialog can identify malformed entries from its own request payload. | -| `devices/rename` | `{configuration, new_name, config_only?: bool}` | `{configuration, job, tail_job}` (config-only / in-place: `{configuration, job: null}`) | Rename device. The online path writes the renamed YAML up-front and queues a **two-job chain**: `job` is the `COMPILE` of the new YAML (remote-eligible via the build scheduler, exactly like an install's) and `tail_job` is the dependent `RENAME` that OTA-flashes the *old* device address on the upload lane and swaps the files on success. A failed / cancelled chain deletes the new YAML so the old device is untouched. `config_only: true` (offline) and in-place renames rewrite the YAML with no jobs. A non-retargetable `esphome.name` (from packages / `!include` / an embedded substitution) refuses with `INVALID_ARGS` on every path. Both record the pre-rename `esphome.name` (as does a hand-edited one), since the running firmware keeps broadcasting it until the next install, and the device's OTA address-cache args are then published under the new `.local` key using that name's cached addresses. A config-only or in-place rename changes nothing and answers `PRECONDITION_FAILED` when the file changed while the rewrite was being validated, `NOT_FOUND` when it was deleted meanwhile, and the usual name-exists `INVALID_ARGS` when another config took the new filename meanwhile. | +| `devices/rename` | `{configuration, new_name, config_only?: bool}` | `{configuration, job, tail_job}` (config-only / in-place: `{configuration, job: null}`) | Rename device. The online path writes the renamed YAML up-front and queues a **two-job chain**: `job` is the `COMPILE` of the new YAML (remote-eligible via the build scheduler, exactly like an install's) and `tail_job` is the dependent `RENAME` that OTA-flashes the *old* device address on the upload lane and swaps the files on success. A failed / cancelled chain deletes the new YAML so the old device is untouched. `config_only: true` (offline) and in-place renames rewrite the YAML with no jobs. A non-retargetable `esphome.name` (from packages / `!include` / an embedded substitution) refuses with `INVALID_ARGS` on every path. Both remember the pre-rename hostname, which the firmware keeps broadcasting until the next install, so a later install still reaches the device (see ARCHITECTURE for the lifecycle). A config-only or in-place rename changes nothing and answers `PRECONDITION_FAILED` when the file changed while the rewrite was being validated, `NOT_FOUND` when it was deleted meanwhile, and the usual name-exists `INVALID_ARGS` when another config took the new filename meanwhile. | | `devices/delete` | `{configuration}` | — | Delete device and associated files. A missing YAML surfaces `not_found` (same as `devices/archive`), not `internal_error`. | | `devices/delete_bulk` | `{configurations: string[]}` | `[{configuration, success, error?}]` | Delete multiple devices | | `devices/archive` | `{configuration}` | — | Soft-delete: move YAML to `/archive/`, wipe build dir, wipe StorageJSON + device-metadata sidecars. Reversible via `devices/unarchive` (cached IP/version/hash refill from the next mDNS broadcast). | diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9d4dc2b000..20bf8f4337 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -265,21 +265,11 @@ firmware/install {configuration} → QUEUED → RUNNING → output... → COMPLE cancelling the tail cascades *up* to its compile. A persisted RENAME with no `depends_on` (pre-decomposition) still runs the fused `esphome rename` CLI on the compile lane. The flash-free branches - (`config_only` for an offline device, and an in-place rename whose target - filename is the device's own) skip the chain, so the firmware keeps - broadcasting the pre-rename hostname. That name is recorded as - `deployed_name` in the device-metadata store and backs the device's OTA - address-cache args, so an install still reaches a device that never - announces its new name (#2730); the record is ignored while another config - owns that name, since its broadcast is then somebody else's. A - hand-edited `esphome.name` records the same way, off the scan's name - change, for a device with build output. A chained flash-free - rename keeps the first record; it clears on a rename back to it, on the - next app flash or completed rename chain (a `--bootloader` upload replaces - no app, so it keeps the record), and when mDNS takes ownership of the - device's own name (the self-heal for a flash from outside the dashboard, - keyed on ownership rather than the state flip so a ping-online device is - covered). + (`config_only` for an offline device, an in-place rename whose target + filename is the device's own, and a hand-edited `esphome.name`) skip the + chain, so the firmware keeps broadcasting the pre-rename hostname; that + name is remembered as `deployed_name` (#2730), covered below with the rest + of the live-state fields. - Plus a **remote build-server pool** — one more consumer (`run_dispatch_loop`) gathered alongside the lane workers. Compiles eligible for a paired server hold here (off the single compile lane) and run concurrently, one per @@ -833,6 +823,8 @@ Per-device metadata is partitioned across two files by *who writes it* and *how * **Identity** (`board_id`, `friendly_name`, `comment`, `labels`, `mac_address`) lives in `/.device-builder.json` alongside the cross-flavor catalog keys (`_labels`, `_remote_build`, `dashboard_id`). Access goes through `SharedSidecarClient` — a thin async wrapper around the existing `helpers/metadata_sidecar.metadata_transaction` (`fcntl.flock` + `_METADATA_LOCK` for cross-flavor RMW safety). Writes are infrequent (user-edited names, scanner-derived `board_id` backfill, first-observation `mac_address`) and run through the transactional path so the `esphome` / `esphome-beta` / `esphome-dev` flavors on a shared `/config/esphome` can't clobber each other. * **Live state** (`ip`, `expected_config_hash`, `deployed_config_hash`, `deployed_version`, `deployed_name`, `api_encryption_active`, `build_size_*`, `regen_failed_*`) lives in `/.device-builder-devices.json`. Access goes through `DeviceMetadataStore` — a `helpers.storage.Store`-backed RAM-canonical dict that debounces writes (2s coalesce) and flushes on shutdown. The store keys on `` rather than `` because each HA-addon flavor compiles its own binaries and observes its own mDNS broadcasts; sharing this state across flavors would let one flavor's running-firmware hash overwrite another's. The file is per-flavor by construction, so no cross-process lock is needed beyond the single-instance startup `flock` that already pins one process per `data_dir`. +`deployed_name` is the odd one out: it is stamped, not observed. A rename that doesn't flash (and a hand-edited `esphome.name`, off the scan's name change, for a device with build output) leaves the firmware answering its old hostname, so that name is recorded and backs the device's OTA address-cache args — published under the new `.local` key, so an install still reaches a device that never announces its new name. It is ignored while another config owns that name, since the broadcast is then somebody else's. A chained flash-free rename keeps the first record; it clears on a rename back to it, on the next app flash or completed rename chain (a `--bootloader` upload replaces no app, so the record stands), and when mDNS takes ownership of the device's own name — keyed on ownership rather than the state flip, so a device held ONLINE by ping is covered. That last one is the self-heal for a flash from outside the dashboard. + The `STORE_FIELDS` frozenset in `controllers/devices/_metadata_store.py` enumerates the live-state field names; `DeviceMetadataBase._persist_device_metadata_async` is the routing dispatcher (anything in `STORE_FIELDS` → store, everything else → shared sidecar). The mDNS hot path (`state_callbacks.on_*`) writes the store directly via `controller._metadata_store.update(...)` / `set_field(...)` — sync RAM mutation on the event loop, debounced disk write on the executor. **Migration from pre-split state.** On first start after the split, `DeviceMetadataStore.async_load()` reads any live-state fields still present in `/.device-builder.json` (older releases stored everything there), writes them to `/.device-builder-devices.json`, then strips them from the shared sidecar — leaving the shared file with only identity + cross-flavor catalog keys. The migration runs through `metadata_transaction` so a concurrent flavor can't race the strip. Crashing between the new-file flush and the shared-file strip leaks duplicate data (the shared sidecar's stale live-state fields are ignored by `_resolve_device_metadata` once the store has them) but no data is lost. Downgrading to a pre-split release after migration loses live state until devices re-broadcast. diff --git a/esphome_device_builder/controllers/devices/_metadata_store.py b/esphome_device_builder/controllers/devices/_metadata_store.py index f9bdd2b9c0..7f4e88891b 100644 --- a/esphome_device_builder/controllers/devices/_metadata_store.py +++ b/esphome_device_builder/controllers/devices/_metadata_store.py @@ -19,6 +19,9 @@ _DEFAULT_SAVE_DELAY = 2.0 +# Read-only miss sentinel for ``get_field``; never mutated. +_EMPTY: dict[str, Any] = {} + # Fields the store owns. Everything else lives in the shared sidecar. STORE_FIELDS: frozenset[str] = frozenset( { @@ -110,6 +113,10 @@ def get(self, filename: str) -> dict[str, Any]: """Return a shallow copy of *filename*'s metadata.""" return dict(self._state.get(filename, {})) + def get_field(self, filename: str, key: str) -> Any: + """Return one field of *filename*'s metadata without copying the entry.""" + return self._state.get(filename, _EMPTY).get(key) + def update( self, filename: str, diff --git a/esphome_device_builder/controllers/devices/controller.py b/esphome_device_builder/controllers/devices/controller.py index 07951cc047..bc32e60d28 100644 --- a/esphome_device_builder/controllers/devices/controller.py +++ b/esphome_device_builder/controllers/devices/controller.py @@ -428,12 +428,19 @@ def get_address_cache_args(self, configuration: str) -> list[str]: loaded = device.loaded_integrations if loaded and "api" not in loaded and "web_server" not in loaded: return [] - deployed_name = self._metadata_store.get(configuration).get("deployed_name", "") - if deployed_name and self._scanner.get_by_name(deployed_name): - # Another config reclaimed the name, so whatever answers to it - # is no longer this device. - deployed_name = "" - return _build_address_cache_args(device, self._state_monitor, deployed_name) + return _build_address_cache_args( + device, self._state_monitor, self._deployed_name(configuration) + ) + + def _deployed_name(self, configuration: str) -> str: + """ + Return the hostname *configuration*'s firmware still answers to, if any. + + Empty while another config owns that name: whatever answers to + it is then somebody else's device, not ours. + """ + deployed_name = self._metadata_store.get_field(configuration, "deployed_name") or "" + return "" if self._devices_by_name(deployed_name) else deployed_name def get_ota_address_cache_args(self, configuration: str, port: str | None) -> list[str]: """Return cache args when ``port == "OTA"`` (or ``None`` for always-OTA flows).""" diff --git a/esphome_device_builder/controllers/devices/firmware_sync.py b/esphome_device_builder/controllers/devices/firmware_sync.py index 0b696eacc2..a6556188ab 100644 --- a/esphome_device_builder/controllers/devices/firmware_sync.py +++ b/esphome_device_builder/controllers/devices/firmware_sync.py @@ -91,9 +91,10 @@ def on_job_completed(controller: DevicesController, event: Event[JobLifecycleDat recompute_hash = job_type in COMPILING_JOB_TYPES flashed = job_type in (JobType.UPLOAD, JobType.INSTALL) if flashed and not job.flash_bootloader: - # The app image now carries the YAML's own name; a bootloader-only - # upload replaces no app, so its record stands. - controller._metadata_store.update(configuration, deployed_name="") + # A bootloader-only upload replaces no app, so its record stands. + # Cleared here, not in the background refresh, so a following job's + # address-cache read can't race it. + controller._clear_deployed_name(configuration) # Routed through the controller's bound delegate so tests # that monkeypatch ``_refresh_after_firmware_job`` on the # instance still intercept. @@ -237,9 +238,8 @@ async def migrate_metadata_then_scan( controller: DevicesController, old_configuration: str, new_configuration: str ) -> None: """Move the renamed device's metadata before the scan rebuilds it.""" - # Same clear as the flashed branch of ``refresh_after_job``, which a - # completed RENAME never reaches. - controller._metadata_store.update(old_configuration, deployed_name="") + # The tail flashed the new name; a completed RENAME reaches no other clear. + controller._clear_deployed_name(old_configuration) await migrate_metadata(controller, old_configuration, new_configuration) await rescan_renamed(controller, new_configuration) diff --git a/esphome_device_builder/controllers/devices/helpers.py b/esphome_device_builder/controllers/devices/helpers.py index 764057047d..2ae67350d8 100644 --- a/esphome_device_builder/controllers/devices/helpers.py +++ b/esphome_device_builder/controllers/devices/helpers.py @@ -479,12 +479,7 @@ def _gates_on_unconfigured_block( def _build_address_cache_args( device: Device, monitor: DeviceStateMonitor | None, deployed_name: str = "" ) -> list[str]: - """ - Build CLI cache args from the IPs we already have for *device*. - - *deployed_name* is the hostname the firmware still answers to after a - flash-free rename; its addresses back the YAML name's key. - """ + """Build CLI cache args from the IPs we have for *device* or *deployed_name*.""" address = device.address if not address: return [] diff --git a/esphome_device_builder/controllers/devices/metadata.py b/esphome_device_builder/controllers/devices/metadata.py index 584593f76f..c15e4849af 100644 --- a/esphome_device_builder/controllers/devices/metadata.py +++ b/esphome_device_builder/controllers/devices/metadata.py @@ -219,16 +219,21 @@ async def _migrate_device_metadata( await self._shared_sidecar.rename(old_configuration, new_configuration) def _stamp_deployed_name(self, configuration: str, *, old_name: str, new_name: str) -> str: - """Record the hostname the firmware still answers to after a flash-free rename. + """ + Record the hostname the firmware answers to; returns it (``""`` if cleared). - A chained rename keeps the existing record; renaming back to it - clears. Returns the recorded name (``""`` when cleared). + A chained rename keeps the existing record; renaming back to it clears. """ - stamped = self._metadata_store.get(configuration).get("deployed_name") or old_name - deployed = "" if stamped == new_name else stamped + current = self._metadata_store.get_field(configuration, "deployed_name") or old_name + deployed = "" if current == new_name else current self._metadata_store.update(configuration, deployed_name=deployed) return deployed + def _clear_deployed_name(self, configuration: str) -> None: + """Forget the recorded hostname; the firmware carries the YAML's own name.""" + if self._metadata_store.get_field(configuration, "deployed_name"): + self._metadata_store.update(configuration, deployed_name="") + async def _clear_volatile_device_metadata(self, configuration: str) -> None: """Clear archive-volatile fields in both stores (keeps identity). diff --git a/esphome_device_builder/controllers/devices/mutations_simple.py b/esphome_device_builder/controllers/devices/mutations_simple.py index a65999ac5f..0f469b13fb 100644 --- a/esphome_device_builder/controllers/devices/mutations_simple.py +++ b/esphome_device_builder/controllers/devices/mutations_simple.py @@ -346,11 +346,10 @@ def _land() -> None: await migrate_metadata(controller, configuration, new_filename) await rescan_renamed(controller, new_filename) # An in-place rescan re-stamps the pre-rename name over a record this - # rename just cleared. Only the clear is re-asserted: the stamp keeps - # an existing record, so a set is never the value that got polluted, - # and re-clearing never overwrites a fresher mDNS or rename value. + # rename just cleared. Only the clear is re-asserted, so this can't + # overwrite a fresher mDNS or concurrent-rename value. if not deployed_name: - controller._metadata_store.update(new_filename, deployed_name="") + controller._clear_deployed_name(new_filename) return {"configuration": new_filename, "job": None} diff --git a/esphome_device_builder/controllers/devices/scan_change.py b/esphome_device_builder/controllers/devices/scan_change.py index 925328f973..6cd4da94e8 100644 --- a/esphome_device_builder/controllers/devices/scan_change.py +++ b/esphome_device_builder/controllers/devices/scan_change.py @@ -152,7 +152,7 @@ def _reconcile_rename( device: Device, previous: Device | None, ) -> None: - """Reconcile importables and per-name monitor state after a name correction.""" + """Record the old hostname, then reconcile importables and per-name monitor state.""" if ( kind not in (ScanChange.UPDATED, ScanChange.RELOADED) or previous is None @@ -160,8 +160,8 @@ def _reconcile_rename( ): return # A hand-edited name strands the firmware like a config-only rename. - # Gated on build output: the cold-start shallow→deep refine lands - # here too, renaming off a filename-stem placeholder. + # Gated on build output: the cold-start refine lands here too, off a + # filename-stem placeholder no firmware answered to. if device.loaded_integrations: controller._stamp_deployed_name( device.configuration, old_name=previous.name, new_name=device.name diff --git a/esphome_device_builder/controllers/devices/state_callbacks.py b/esphome_device_builder/controllers/devices/state_callbacks.py index e804b4d188..8a621f8043 100644 --- a/esphome_device_builder/controllers/devices/state_callbacks.py +++ b/esphome_device_builder/controllers/devices/state_callbacks.py @@ -80,15 +80,14 @@ def on_state_change( def on_source_change(controller: DevicesController, name: str, source: ReachabilitySource) -> None: - """Update ``active_source`` and fire DEVICE_UPDATED; runtime-only, not persisted.""" + """Update ``active_source`` and fire DEVICE_UPDATED; also clears ``deployed_name``.""" for device in controller._devices_by_name(name): if device.runtime_state.active_source == source: continue if source is ReachabilitySource.MDNS: # An announce under the device's own name proves the firmware - # carries it. Ownership, not the state flip: a ping-online - # device stays ONLINE, so only the source moves. - controller._metadata_store.update(device.configuration, deployed_name="") + # carries it; ownership moves even when the state doesn't. + controller._clear_deployed_name(device.configuration) device.runtime_state.active_source = source controller._fire_device_updated(device) diff --git a/tests/conftest.py b/tests/conftest.py index fd4c894708..b502c0d59f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -957,6 +957,16 @@ async def release_and_drain_advertise(db: Any) -> None: # --------------------------------------------------------------------------- +def attach_metadata_store(controller: Any, directory: Path) -> None: + """Wire a real ``DeviceMetadataStore`` (and its shutdown list) onto *controller*.""" + controller._shutdown_callbacks = [] + controller._metadata_store = DeviceMetadataStore( + config_dir=directory, + data_dir=directory, + shutdown_register=controller._shutdown_callbacks.append, + ) + + def make_devices_controller_with_bus( devices: list[Device], *, @@ -1009,12 +1019,7 @@ def make_devices_controller_with_bus( tmp_dir_obj = _tempfile.TemporaryDirectory(prefix="dmstore_") tmp_dir = Path(tmp_dir_obj.name) controller._tmpdir = tmp_dir_obj # keep alive - controller._shutdown_callbacks = [] - controller._metadata_store = DeviceMetadataStore( - config_dir=tmp_dir, - data_dir=tmp_dir, - shutdown_register=controller._shutdown_callbacks.append, - ) + attach_metadata_store(controller, tmp_dir) controller._shared_sidecar = SharedSidecarClient(tmp_dir) return controller, captured diff --git a/tests/controllers/firmware/test_address_cache.py b/tests/controllers/firmware/test_address_cache.py index 09c4d4a376..e6152220a4 100644 --- a/tests/controllers/firmware/test_address_cache.py +++ b/tests/controllers/firmware/test_address_cache.py @@ -198,7 +198,10 @@ def test_deployed_name_ignored_for_non_local_address() -> None: def _devices_controller_with( - *devices: Device, deployed_name: str = "", names_taken: tuple[str, ...] = () + *devices: Device, + monitor: RecordingStateMonitor | None = None, + deployed_name: str = "", + names_taken: tuple[str, ...] = (), ) -> Any: """Build a thin DevicesController shell with a stubbed scanner + monitor. @@ -206,15 +209,16 @@ def _devices_controller_with( lookup, the state monitor's cached-addresses lookup, the metadata store's ``deployed_name`` record, and the device's ``loaded_integrations`` field — keep the rest of the controller out - of the test surface. + of the test surface. ``names_taken`` seeds the name index the + reclaimed-record guard consults. """ controller = DevicesController.__new__(DevicesController) scanner = RecordingScanner(devices_by_name={name: [object()] for name in names_taken}) scanner.devices = list(devices) controller._scanner = scanner - controller._state_monitor = _monitor(["192.168.1.50"]) + controller._state_monitor = monitor if monitor is not None else _monitor(["192.168.1.50"]) controller._metadata_store = SimpleNamespace( - get=lambda _configuration: {"deployed_name": deployed_name} + get_field=lambda _configuration, _key: deployed_name ) return controller @@ -303,18 +307,29 @@ def test_get_address_cache_args_drops_a_reclaimed_deployed_name() -> None: """A config that took the old name owns its broadcast; fall back to the IP.""" controller = _devices_controller_with( _device(loaded_integrations=["api"], ip="192.168.1.99"), + monitor=RecordingStateMonitor(cached_addresses={"asistente.local": ["10.0.0.1"]}), deployed_name="asistente", names_taken=("asistente",), ) - controller._state_monitor = RecordingStateMonitor( - cached_addresses={"asistente.local": ["10.0.0.1"]} - ) args = controller.get_address_cache_args("kitchen.yaml") assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.99"] +def test_get_address_cache_args_reads_the_deployed_name_record() -> None: + """The controller feeds the store's record into the cache-args build.""" + controller = _devices_controller_with( + _device(loaded_integrations=["api"]), + monitor=RecordingStateMonitor(cached_addresses={"asistente.local": ["192.168.1.50"]}), + deployed_name="asistente", + ) + + args = controller.get_address_cache_args("kitchen.yaml") + + assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.50"] + + def test_get_ota_address_cache_args_returns_cache_for_ota_port() -> None: """Strict ``port == "OTA"`` → delegate to ``get_address_cache_args``.""" controller = _devices_controller_with(_device()) @@ -339,20 +354,6 @@ def test_get_ota_address_cache_args_empty_for_missing_port() -> None: assert controller.get_ota_address_cache_args("kitchen.yaml", "") == [] -def test_get_address_cache_args_reads_the_deployed_name_record() -> None: - """The controller feeds the store's record into the cache-args build.""" - controller = _devices_controller_with( - _device(loaded_integrations=["api"]), deployed_name="asistente" - ) - controller._state_monitor = RecordingStateMonitor( - cached_addresses={"asistente.local": ["192.168.1.50"]} - ) - - args = controller.get_address_cache_args("kitchen.yaml") - - assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.50"] - - def test_get_ota_address_cache_args_none_is_always_ota() -> None: """``port=None`` skips the OTA gate — for always-OTA flows like ``rename``.""" controller = _devices_controller_with(_device()) diff --git a/tests/controllers/firmware/test_refresh.py b/tests/controllers/firmware/test_refresh.py index 41e90c0e32..37297f4391 100644 --- a/tests/controllers/firmware/test_refresh.py +++ b/tests/controllers/firmware/test_refresh.py @@ -39,7 +39,6 @@ ScanChange, ) from esphome_device_builder.controllers.devices import DevicesController, firmware_sync -from esphome_device_builder.controllers.devices._metadata_store import DeviceMetadataStore from esphome_device_builder.helpers.event_bus import Event from esphome_device_builder.models import ( Device, @@ -51,7 +50,7 @@ ) from tests._recording_scanner import RecordingScanner from tests._storage_fixtures import write_storage_json -from tests.conftest import make_device +from tests.conftest import attach_metadata_store, make_device def _stub_metadata(_config_dir: object, _filename: object) -> DeviceFileMetadata: @@ -145,12 +144,7 @@ async def _noop() -> None: # without needing the full worker lifecycle. controller._build_size = MagicMock() controller._refresh_after_firmware_job = _capturing_refresh # type: ignore[method-assign] - controller._shutdown_callbacks = [] - controller._metadata_store = DeviceMetadataStore( - config_dir=tmp_path, - data_dir=tmp_path, - shutdown_register=controller._shutdown_callbacks.append, - ) + attach_metadata_store(controller, tmp_path) return controller, captured @@ -332,12 +326,7 @@ async def _fake_compute(_path: Path) -> str | None: controller._db = db controller._scanner = RecordingScanner() controller._build_size = MagicMock() - controller._shutdown_callbacks = [] - controller._metadata_store = DeviceMetadataStore( - config_dir=tmp_path, - data_dir=tmp_path, - shutdown_register=controller._shutdown_callbacks.append, - ) + attach_metadata_store(controller, tmp_path) await controller._refresh_after_firmware_job("kitchen.yaml", recompute_hash=True, flashed=False) @@ -366,15 +355,9 @@ async def _fake_compute(_path: Path) -> str | None: controller._db = db controller._scanner = RecordingScanner() controller._build_size = MagicMock() - controller._shutdown_callbacks = [] tmp_dir_obj = _tempfile.TemporaryDirectory(prefix="dmstore_") - tmp_dir = Path(tmp_dir_obj.name) controller._tmpdir = tmp_dir_obj # keep alive - controller._metadata_store = DeviceMetadataStore( - config_dir=tmp_dir, - data_dir=tmp_dir, - shutdown_register=controller._shutdown_callbacks.append, - ) + attach_metadata_store(controller, Path(tmp_dir_obj.name)) await controller._refresh_after_firmware_job("kitchen.yaml", recompute_hash=True, flashed=False) @@ -473,15 +456,9 @@ def _apply_version(name: str, version: str) -> bool: controller._db = db controller._scanner = scanner controller._state_monitor = state_monitor - controller._shutdown_callbacks = [] tmp_dir_obj = _tempfile.TemporaryDirectory(prefix="dmstore_") - tmp_dir = Path(tmp_dir_obj.name) controller._tmpdir = tmp_dir_obj # keep alive - controller._metadata_store = DeviceMetadataStore( - config_dir=tmp_dir, - data_dir=tmp_dir, - shutdown_register=controller._shutdown_callbacks.append, - ) + attach_metadata_store(controller, Path(tmp_dir_obj.name)) return controller, fired From 72a5693a7c50a930954afa74f7c6eea3733bc4fc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 23:14:56 +0100 Subject: [PATCH 09/15] Clear the record above the active_source dedupe A same-path rename carries runtime_state.active_source forward whole, so after an in-place or hand-edited rename the row still reads mdns while the ledger for the new name is unknown. The healing announce flips the ledger but not the row, so the clear below the dedupe never ran; the out-of-band flash it exists for was the case it missed. --- docs/ARCHITECTURE.md | 2 +- .../controllers/devices/state_callbacks.py | 8 +++++--- tests/test_device_state_event.py | 11 +++++++++++ 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 20bf8f4337..d10409df36 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -823,7 +823,7 @@ Per-device metadata is partitioned across two files by *who writes it* and *how * **Identity** (`board_id`, `friendly_name`, `comment`, `labels`, `mac_address`) lives in `/.device-builder.json` alongside the cross-flavor catalog keys (`_labels`, `_remote_build`, `dashboard_id`). Access goes through `SharedSidecarClient` — a thin async wrapper around the existing `helpers/metadata_sidecar.metadata_transaction` (`fcntl.flock` + `_METADATA_LOCK` for cross-flavor RMW safety). Writes are infrequent (user-edited names, scanner-derived `board_id` backfill, first-observation `mac_address`) and run through the transactional path so the `esphome` / `esphome-beta` / `esphome-dev` flavors on a shared `/config/esphome` can't clobber each other. * **Live state** (`ip`, `expected_config_hash`, `deployed_config_hash`, `deployed_version`, `deployed_name`, `api_encryption_active`, `build_size_*`, `regen_failed_*`) lives in `/.device-builder-devices.json`. Access goes through `DeviceMetadataStore` — a `helpers.storage.Store`-backed RAM-canonical dict that debounces writes (2s coalesce) and flushes on shutdown. The store keys on `` rather than `` because each HA-addon flavor compiles its own binaries and observes its own mDNS broadcasts; sharing this state across flavors would let one flavor's running-firmware hash overwrite another's. The file is per-flavor by construction, so no cross-process lock is needed beyond the single-instance startup `flock` that already pins one process per `data_dir`. -`deployed_name` is the odd one out: it is stamped, not observed. A rename that doesn't flash (and a hand-edited `esphome.name`, off the scan's name change, for a device with build output) leaves the firmware answering its old hostname, so that name is recorded and backs the device's OTA address-cache args — published under the new `.local` key, so an install still reaches a device that never announces its new name. It is ignored while another config owns that name, since the broadcast is then somebody else's. A chained flash-free rename keeps the first record; it clears on a rename back to it, on the next app flash or completed rename chain (a `--bootloader` upload replaces no app, so the record stands), and when mDNS takes ownership of the device's own name — keyed on ownership rather than the state flip, so a device held ONLINE by ping is covered. That last one is the self-heal for a flash from outside the dashboard. +`deployed_name` is the odd one out: it is stamped, not observed. A rename that doesn't flash (and a hand-edited `esphome.name`, off the scan's name change, for a device with build output) leaves the firmware answering its old hostname, so that name is recorded and backs the device's OTA address-cache args — published under the new `.local` key, so an install still reaches a device that never announces its new name. It is ignored while another config owns that name, since the broadcast is then somebody else's. A chained flash-free rename keeps the first record; it clears on a rename back to it, on the next app flash or completed rename chain (a `--bootloader` upload replaces no app, so the record stands), and on every mDNS ownership notification for the device's own name — keyed on the notification rather than on a state or source flip, since a device held ONLINE by ping never flips state and a same-path rename carries a stale `active_source` forward. That last one is the self-heal for a flash from outside the dashboard. The `STORE_FIELDS` frozenset in `controllers/devices/_metadata_store.py` enumerates the live-state field names; `DeviceMetadataBase._persist_device_metadata_async` is the routing dispatcher (anything in `STORE_FIELDS` → store, everything else → shared sidecar). The mDNS hot path (`state_callbacks.on_*`) writes the store directly via `controller._metadata_store.update(...)` / `set_field(...)` — sync RAM mutation on the event loop, debounced disk write on the executor. diff --git a/esphome_device_builder/controllers/devices/state_callbacks.py b/esphome_device_builder/controllers/devices/state_callbacks.py index 8a621f8043..bb4fb996b0 100644 --- a/esphome_device_builder/controllers/devices/state_callbacks.py +++ b/esphome_device_builder/controllers/devices/state_callbacks.py @@ -82,12 +82,14 @@ def on_state_change( def on_source_change(controller: DevicesController, name: str, source: ReachabilitySource) -> None: """Update ``active_source`` and fire DEVICE_UPDATED; also clears ``deployed_name``.""" for device in controller._devices_by_name(name): - if device.runtime_state.active_source == source: - continue if source is ReachabilitySource.MDNS: # An announce under the device's own name proves the firmware - # carries it; ownership moves even when the state doesn't. + # carries it. Above the dedupe: a same-path rename carries the + # pre-rename ``active_source`` forward, so the row may already + # read mdns while the ledger for the new name doesn't. controller._clear_deployed_name(device.configuration) + if device.runtime_state.active_source == source: + continue device.runtime_state.active_source = source controller._fire_device_updated(device) diff --git a/tests/test_device_state_event.py b/tests/test_device_state_event.py index 685ce7d26e..a63b1a40ae 100644 --- a/tests/test_device_state_event.py +++ b/tests/test_device_state_event.py @@ -92,3 +92,14 @@ async def test_ping_ownership_keeps_the_deployed_name() -> None: ctrl._on_source_change("kitchen", ReachabilitySource.PING) assert ctrl._metadata_store.get("kitchen.yaml")["deployed_name"] == "asistente" + + +async def test_mdns_ownership_clears_through_a_stale_active_source() -> None: + """A same-path rename carries ``active_source`` forward as mdns (#2730).""" + device = make_device(address="", active_source=ReachabilitySource.MDNS) + ctrl, _ = make_devices_controller_with_bus([device]) + ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + + ctrl._on_source_change("kitchen", ReachabilitySource.MDNS) + + assert "deployed_name" not in ctrl._metadata_store.get("kitchen.yaml") From 0ee2d61cb428f0bff8f143c0ab1f69a3a928aefd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 23:42:42 +0100 Subject: [PATCH 10/15] Let the recorded hostname identify the device to the api reviver Without this the reviver dials the persisted IP, reads the firmware's pre-rename name, calls the lease stale and wipes device.ip, which is the fallback the redirect leans on when mDNS is dark. Worse, the device stays OFFLINE, so enqueue_install_or_defer turns the install into a compile-only job armed for a wake that never comes; the record only pays off if an OTA actually runs. Accepting the recorded name revives it under ping and lets the install through. Also only heal off an mDNS announce when the name maps to one config, and move the deployed_name reader onto the Device row and down into the private section. --- docs/ARCHITECTURE.md | 2 +- .../controllers/_device_scanner.py | 3 +++ .../_device_state_monitor/api_reviver.py | 4 +++- .../controllers/devices/controller.py | 23 ++++++++----------- .../controllers/devices/metadata.py | 1 + .../controllers/devices/state_callbacks.py | 14 ++++++----- .../helpers/device_yaml/_loading.py | 2 ++ esphome_device_builder/models/devices.py | 4 ++++ .../firmware/test_address_cache.py | 11 ++------- tests/test_api_reviver.py | 14 +++++++++++ tests/test_device_state_event.py | 12 ++++++++++ 11 files changed, 60 insertions(+), 30 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index d10409df36..fa4e055561 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -823,7 +823,7 @@ Per-device metadata is partitioned across two files by *who writes it* and *how * **Identity** (`board_id`, `friendly_name`, `comment`, `labels`, `mac_address`) lives in `/.device-builder.json` alongside the cross-flavor catalog keys (`_labels`, `_remote_build`, `dashboard_id`). Access goes through `SharedSidecarClient` — a thin async wrapper around the existing `helpers/metadata_sidecar.metadata_transaction` (`fcntl.flock` + `_METADATA_LOCK` for cross-flavor RMW safety). Writes are infrequent (user-edited names, scanner-derived `board_id` backfill, first-observation `mac_address`) and run through the transactional path so the `esphome` / `esphome-beta` / `esphome-dev` flavors on a shared `/config/esphome` can't clobber each other. * **Live state** (`ip`, `expected_config_hash`, `deployed_config_hash`, `deployed_version`, `deployed_name`, `api_encryption_active`, `build_size_*`, `regen_failed_*`) lives in `/.device-builder-devices.json`. Access goes through `DeviceMetadataStore` — a `helpers.storage.Store`-backed RAM-canonical dict that debounces writes (2s coalesce) and flushes on shutdown. The store keys on `` rather than `` because each HA-addon flavor compiles its own binaries and observes its own mDNS broadcasts; sharing this state across flavors would let one flavor's running-firmware hash overwrite another's. The file is per-flavor by construction, so no cross-process lock is needed beyond the single-instance startup `flock` that already pins one process per `data_dir`. -`deployed_name` is the odd one out: it is stamped, not observed. A rename that doesn't flash (and a hand-edited `esphome.name`, off the scan's name change, for a device with build output) leaves the firmware answering its old hostname, so that name is recorded and backs the device's OTA address-cache args — published under the new `.local` key, so an install still reaches a device that never announces its new name. It is ignored while another config owns that name, since the broadcast is then somebody else's. A chained flash-free rename keeps the first record; it clears on a rename back to it, on the next app flash or completed rename chain (a `--bootloader` upload replaces no app, so the record stands), and on every mDNS ownership notification for the device's own name — keyed on the notification rather than on a state or source flip, since a device held ONLINE by ping never flips state and a same-path rename carries a stale `active_source` forward. That last one is the self-heal for a flash from outside the dashboard. +`deployed_name` is the odd one out: it is stamped, not observed. A rename that doesn't flash (and a hand-edited `esphome.name`, off the scan's name change, for a device with build output) leaves the firmware answering its old hostname, so that name is recorded and backs the device's OTA address-cache args — published under the new `.local` key, so an install still reaches a device that never announces its new name. It is ignored while another config owns that name, since the broadcast is then somebody else's. A chained flash-free rename keeps the first record; it clears on a rename back to it, on the next app flash or completed rename chain (a `--bootloader` upload replaces no app, so the record stands), and when mDNS takes ownership of the device's own name, provided that name maps to one config (siblings sharing a name share the broadcast, so it proves nothing about which was flashed). That last clear sits above the row's `active_source` dedupe, since a same-path rename carries a stale `mdns` value forward. The record also counts as identity for the API reviver, which would otherwise read the firmware's pre-rename name as a re-leased IP and wipe the persisted address the redirect falls back on. That last one is the self-heal for a flash from outside the dashboard. The `STORE_FIELDS` frozenset in `controllers/devices/_metadata_store.py` enumerates the live-state field names; `DeviceMetadataBase._persist_device_metadata_async` is the routing dispatcher (anything in `STORE_FIELDS` → store, everything else → shared sidecar). The mDNS hot path (`state_callbacks.on_*`) writes the store directly via `controller._metadata_store.update(...)` / `set_field(...)` — sync RAM mutation on the event loop, debounced disk write on the executor. diff --git a/esphome_device_builder/controllers/_device_scanner.py b/esphome_device_builder/controllers/_device_scanner.py index 6c3a9d4ac0..f023674ce7 100644 --- a/esphome_device_builder/controllers/_device_scanner.py +++ b/esphome_device_builder/controllers/_device_scanner.py @@ -63,6 +63,8 @@ class DeviceFileMetadata(NamedTuple): # absent (plaintext-confirmed); ``None`` means not yet # broadcast. api_encryption_active: str | None = None + # Pre-rename hostname the firmware still answers to, if any. + deployed_name: str = "" class ScanChange(StrEnum): @@ -496,6 +498,7 @@ def _load_devices(self, paths: set[Path], *, shallow: bool = False) -> dict[Path deployed_version=metadata.deployed_version, queued_update=metadata.queued_update, api_encryption_active=metadata.api_encryption_active, + deployed_name=metadata.deployed_name, previous=self._index.by_path.get(path), shallow=shallow, ) diff --git a/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py b/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py index d29ea179e9..61a50e8cc5 100644 --- a/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py +++ b/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py @@ -217,7 +217,9 @@ async def _verify_and_revive(self, device: Device, ip: str, rtt: float) -> None: ) self._record_dial_failure(key) return - if reported != device.name: + # A device renamed without a flash still answers its recorded + # pre-rename name, so that name identifies it just as well. + if reported not in (device.name, device.deployed_name): # Whatever holds the lease now is a different device; the # persisted IP is proven stale — invalidate it so neither the # reviver nor the OTA cache trusts it again. ``reported`` is diff --git a/esphome_device_builder/controllers/devices/controller.py b/esphome_device_builder/controllers/devices/controller.py index bc32e60d28..a1821d5dce 100644 --- a/esphome_device_builder/controllers/devices/controller.py +++ b/esphome_device_builder/controllers/devices/controller.py @@ -428,19 +428,7 @@ def get_address_cache_args(self, configuration: str) -> list[str]: loaded = device.loaded_integrations if loaded and "api" not in loaded and "web_server" not in loaded: return [] - return _build_address_cache_args( - device, self._state_monitor, self._deployed_name(configuration) - ) - - def _deployed_name(self, configuration: str) -> str: - """ - Return the hostname *configuration*'s firmware still answers to, if any. - - Empty while another config owns that name: whatever answers to - it is then somebody else's device, not ours. - """ - deployed_name = self._metadata_store.get_field(configuration, "deployed_name") or "" - return "" if self._devices_by_name(deployed_name) else deployed_name + return _build_address_cache_args(device, self._state_monitor, self._deployed_name(device)) def get_ota_address_cache_args(self, configuration: str, port: str | None) -> list[str]: """Return cache args when ``port == "OTA"`` (or ``None`` for always-OTA flows).""" @@ -1340,6 +1328,15 @@ async def _sync_deployed_state_after_flash(self, configuration: str) -> None: def _schedule_version_reprobe(self, configuration: str) -> None: firmware_sync.schedule_version_reprobe(self, configuration) + def _deployed_name(self, device: Device) -> str: + """ + Return the hostname *device*'s firmware still answers to, if any. + + Empty while another config owns that name: whatever answers to + it is then somebody else's device, not ours. + """ + return "" if self._devices_by_name(device.deployed_name) else device.deployed_name + def _cancel_reprobe_timers(self) -> None: """Cancel any pending post-flash re-probe timers.""" for handle in self._reprobe_timers.values(): diff --git a/esphome_device_builder/controllers/devices/metadata.py b/esphome_device_builder/controllers/devices/metadata.py index c15e4849af..52337deb6a 100644 --- a/esphome_device_builder/controllers/devices/metadata.py +++ b/esphome_device_builder/controllers/devices/metadata.py @@ -139,6 +139,7 @@ def _resolve_device_metadata( deployed_version=deployed_version, queued_update=queued_update, api_encryption_active=api_encryption_active, + deployed_name=str(store_md.get("deployed_name", "")), ) def _make_metadata_resolver(self) -> MetadataResolver: diff --git a/esphome_device_builder/controllers/devices/state_callbacks.py b/esphome_device_builder/controllers/devices/state_callbacks.py index bb4fb996b0..78f540c095 100644 --- a/esphome_device_builder/controllers/devices/state_callbacks.py +++ b/esphome_device_builder/controllers/devices/state_callbacks.py @@ -81,12 +81,14 @@ def on_state_change( def on_source_change(controller: DevicesController, name: str, source: ReachabilitySource) -> None: """Update ``active_source`` and fire DEVICE_UPDATED; also clears ``deployed_name``.""" - for device in controller._devices_by_name(name): - if source is ReachabilitySource.MDNS: - # An announce under the device's own name proves the firmware - # carries it. Above the dedupe: a same-path rename carries the - # pre-rename ``active_source`` forward, so the row may already - # read mdns while the ledger for the new name doesn't. + devices = controller._devices_by_name(name) + # An announce under the device's own name proves the firmware carries + # it. Above the dedupe, since a same-path rename carries the pre-rename + # ``active_source`` forward; and only when the name maps to one config, + # since siblings sharing a name share the broadcast that proves it. + heals_identity = source is ReachabilitySource.MDNS and len(devices) == 1 + for device in devices: + if heals_identity: controller._clear_deployed_name(device.configuration) if device.runtime_state.active_source == source: continue diff --git a/esphome_device_builder/helpers/device_yaml/_loading.py b/esphome_device_builder/helpers/device_yaml/_loading.py index d307db99b8..63b5c7ee48 100644 --- a/esphome_device_builder/helpers/device_yaml/_loading.py +++ b/esphome_device_builder/helpers/device_yaml/_loading.py @@ -79,6 +79,7 @@ def load_device_from_storage( deployed_version: str = "", queued_update: bool = False, api_encryption_active: str | None = None, + deployed_name: str = "", previous: Device | None = None, shallow: bool = False, ) -> Device: @@ -353,6 +354,7 @@ def load_device_from_storage( address=(storage.address if storage and storage.address else f"{fallback_name}.local"), content_fingerprint=extract_config_content_fingerprint(yaml_content), ip=ip, + deployed_name=deployed_name, web_port=storage.web_port if storage else None, current_version=const.__version__, expected_config_hash=expected_config_hash, diff --git a/esphome_device_builder/models/devices.py b/esphome_device_builder/models/devices.py index 49ff9b04e9..b87197f381 100644 --- a/esphome_device_builder/models/devices.py +++ b/esphome_device_builder/models/devices.py @@ -174,6 +174,10 @@ class Device(DashboardModel): # Survives a confirmed mDNS Removed (only ``ip_addresses`` clears); # dropped only by the reviver's identity-verified invalidation. ip: str = "" + # Hostname the running firmware still answers to after a rename that + # didn't flash; empty once the YAML's own name is deployed. Backs the + # OTA address cache and counts as identity for the api reviver. + deployed_name: str = "" web_port: int | None = None current_version: str = "" # 8-char hex hash of the YAML as last successfully compiled. diff --git a/tests/controllers/firmware/test_address_cache.py b/tests/controllers/firmware/test_address_cache.py index e6152220a4..ecfbffcabb 100644 --- a/tests/controllers/firmware/test_address_cache.py +++ b/tests/controllers/firmware/test_address_cache.py @@ -9,7 +9,6 @@ from __future__ import annotations -from types import SimpleNamespace from typing import Any from unittest.mock import MagicMock @@ -200,7 +199,6 @@ def test_deployed_name_ignored_for_non_local_address() -> None: def _devices_controller_with( *devices: Device, monitor: RecordingStateMonitor | None = None, - deployed_name: str = "", names_taken: tuple[str, ...] = (), ) -> Any: """Build a thin DevicesController shell with a stubbed scanner + monitor. @@ -217,9 +215,6 @@ def _devices_controller_with( scanner.devices = list(devices) controller._scanner = scanner controller._state_monitor = monitor if monitor is not None else _monitor(["192.168.1.50"]) - controller._metadata_store = SimpleNamespace( - get_field=lambda _configuration, _key: deployed_name - ) return controller @@ -306,9 +301,8 @@ def test_get_address_cache_args_filename_differs_from_device_name() -> None: def test_get_address_cache_args_drops_a_reclaimed_deployed_name() -> None: """A config that took the old name owns its broadcast; fall back to the IP.""" controller = _devices_controller_with( - _device(loaded_integrations=["api"], ip="192.168.1.99"), + _device(loaded_integrations=["api"], ip="192.168.1.99", deployed_name="asistente"), monitor=RecordingStateMonitor(cached_addresses={"asistente.local": ["10.0.0.1"]}), - deployed_name="asistente", names_taken=("asistente",), ) @@ -320,9 +314,8 @@ def test_get_address_cache_args_drops_a_reclaimed_deployed_name() -> None: def test_get_address_cache_args_reads_the_deployed_name_record() -> None: """The controller feeds the store's record into the cache-args build.""" controller = _devices_controller_with( - _device(loaded_integrations=["api"]), + _device(loaded_integrations=["api"], deployed_name="asistente"), monitor=RecordingStateMonitor(cached_addresses={"asistente.local": ["192.168.1.50"]}), - deployed_name="asistente", ) args = controller.get_address_cache_args("kitchen.yaml") diff --git a/tests/test_api_reviver.py b/tests/test_api_reviver.py index d7fbb9d45b..bf45ed8fd3 100644 --- a/tests/test_api_reviver.py +++ b/tests/test_api_reviver.py @@ -353,6 +353,20 @@ async def test_stale_verification_re_dials() -> None: # ---------------------------------------------------------------------- +async def test_recorded_pre_rename_name_revives_instead_of_invalidating() -> None: + """A flash-free rename leaves the firmware on its recorded name (#2730).""" + device = make_stuck_offline_device(deployed_name="asistente") + _monitor, callbacks, src = _reviver( + [device], worker_result={**_WORKER_MATCH, "name": "asistente"} + ) + + await src._sweep() + + assert callbacks.calls_for("on_persisted_ip_invalidated") == [] + assert device.ip == "192.168.1.50" + assert ("on_state_change", "kitchen", DeviceState.ONLINE, "ping") in callbacks.calls + + async def test_name_mismatch_invalidates_the_persisted_ip() -> None: device = make_stuck_offline_device() monitor, callbacks, src = _reviver( diff --git a/tests/test_device_state_event.py b/tests/test_device_state_event.py index a63b1a40ae..a992b1e181 100644 --- a/tests/test_device_state_event.py +++ b/tests/test_device_state_event.py @@ -103,3 +103,15 @@ async def test_mdns_ownership_clears_through_a_stale_active_source() -> None: ctrl._on_source_change("kitchen", ReachabilitySource.MDNS) assert "deployed_name" not in ctrl._metadata_store.get("kitchen.yaml") + + +async def test_mdns_of_a_shared_name_keeps_both_deployed_names() -> None: + """Siblings share one broadcast, so it can't prove which one was flashed.""" + first = make_device(configuration="kitchen.yaml", address="") + second = make_device(configuration="kitchen (1).yaml", address="") + ctrl, _ = make_devices_controller_with_bus([first, second]) + ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + + ctrl._on_source_change("kitchen", ReachabilitySource.MDNS) + + assert ctrl._metadata_store.get("kitchen.yaml")["deployed_name"] == "asistente" From 7fe579e96a2e71d0999c0bb048631689cff410f9 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 21 Sep 2026 23:52:22 +0100 Subject: [PATCH 11/15] Keep the record in process and tighten its read paths Mark deployed_name serialize-omit: both readers are backend, nothing on the frontend consumes it, and shipping it would make an internal record a de-facto wire contract. Apply the reclaimed-name rule in the reviver too, so the rule the docs state as a property of the record holds at both readers rather than just one. Hoist the mDNS clear out of a loop it can only traverse once, fold the address fallback into the lookup shape above it, short-circuit the empty record before the bucket lookup, coerce the stamp like the load path does, and collapse the source-change tests into one table. --- docs/ARCHITECTURE.md | 6 +- .../_device_state_monitor/api_reviver.py | 8 +- .../controllers/devices/controller.py | 10 +-- .../controllers/devices/helpers.py | 8 +- .../controllers/devices/metadata.py | 5 +- .../controllers/devices/state_callbacks.py | 13 ++-- esphome_device_builder/models/devices.py | 2 +- .../firmware/test_address_cache.py | 11 ++- tests/test_api_reviver.py | 14 ++++ tests/test_device_state_event.py | 76 +++++++++---------- 10 files changed, 82 insertions(+), 71 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index fa4e055561..f47c71085c 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -823,7 +823,11 @@ Per-device metadata is partitioned across two files by *who writes it* and *how * **Identity** (`board_id`, `friendly_name`, `comment`, `labels`, `mac_address`) lives in `/.device-builder.json` alongside the cross-flavor catalog keys (`_labels`, `_remote_build`, `dashboard_id`). Access goes through `SharedSidecarClient` — a thin async wrapper around the existing `helpers/metadata_sidecar.metadata_transaction` (`fcntl.flock` + `_METADATA_LOCK` for cross-flavor RMW safety). Writes are infrequent (user-edited names, scanner-derived `board_id` backfill, first-observation `mac_address`) and run through the transactional path so the `esphome` / `esphome-beta` / `esphome-dev` flavors on a shared `/config/esphome` can't clobber each other. * **Live state** (`ip`, `expected_config_hash`, `deployed_config_hash`, `deployed_version`, `deployed_name`, `api_encryption_active`, `build_size_*`, `regen_failed_*`) lives in `/.device-builder-devices.json`. Access goes through `DeviceMetadataStore` — a `helpers.storage.Store`-backed RAM-canonical dict that debounces writes (2s coalesce) and flushes on shutdown. The store keys on `` rather than `` because each HA-addon flavor compiles its own binaries and observes its own mDNS broadcasts; sharing this state across flavors would let one flavor's running-firmware hash overwrite another's. The file is per-flavor by construction, so no cross-process lock is needed beyond the single-instance startup `flock` that already pins one process per `data_dir`. -`deployed_name` is the odd one out: it is stamped, not observed. A rename that doesn't flash (and a hand-edited `esphome.name`, off the scan's name change, for a device with build output) leaves the firmware answering its old hostname, so that name is recorded and backs the device's OTA address-cache args — published under the new `.local` key, so an install still reaches a device that never announces its new name. It is ignored while another config owns that name, since the broadcast is then somebody else's. A chained flash-free rename keeps the first record; it clears on a rename back to it, on the next app flash or completed rename chain (a `--bootloader` upload replaces no app, so the record stands), and when mDNS takes ownership of the device's own name, provided that name maps to one config (siblings sharing a name share the broadcast, so it proves nothing about which was flashed). That last clear sits above the row's `active_source` dedupe, since a same-path rename carries a stale `mdns` value forward. The record also counts as identity for the API reviver, which would otherwise read the firmware's pre-rename name as a re-leased IP and wipe the persisted address the redirect falls back on. That last one is the self-heal for a flash from outside the dashboard. +`deployed_name` is the odd one out: it is stamped, not observed. A rename that doesn't flash (config-only, in-place, or a hand-edited `esphome.name` on a device with build output) leaves the firmware answering its old hostname, so that name is recorded and backs the device's OTA address-cache args — published under the new `.local` key, so an install still reaches a device that never announces its new name. Both readers (the cache-args build and the API reviver, which would otherwise read the firmware's pre-rename name as a re-leased IP and wipe the persisted address) ignore it while another config owns that name, since the answer is then somebody else's. It clears on: + +* the next app flash (a `--bootloader` upload replaces no app, so the record stands) or a completed rename chain; +* a rename back to the recorded name; +* mDNS taking ownership of the device's own name, when that name maps to one config — the self-heal for a flash from outside the dashboard. The `STORE_FIELDS` frozenset in `controllers/devices/_metadata_store.py` enumerates the live-state field names; `DeviceMetadataBase._persist_device_metadata_async` is the routing dispatcher (anything in `STORE_FIELDS` → store, everything else → shared sidecar). The mDNS hot path (`state_callbacks.on_*`) writes the store directly via `controller._metadata_store.update(...)` / `set_field(...)` — sync RAM mutation on the event loop, debounced disk write on the executor. diff --git a/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py b/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py index 61a50e8cc5..638d56adc4 100644 --- a/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py +++ b/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py @@ -218,8 +218,12 @@ async def _verify_and_revive(self, device: Device, ip: str, rtt: float) -> None: self._record_dial_failure(key) return # A device renamed without a flash still answers its recorded - # pre-rename name, so that name identifies it just as well. - if reported not in (device.name, device.deployed_name): + # pre-rename name, so that name identifies it too — unless another + # config has since claimed it, when the answer isn't ours to read. + recorded = device.deployed_name + if recorded and monitor._get_devices_by_name(recorded): + recorded = "" + if reported not in (device.name, recorded): # Whatever holds the lease now is a different device; the # persisted IP is proven stale — invalidate it so neither the # reviver nor the OTA cache trusts it again. ``reported`` is diff --git a/esphome_device_builder/controllers/devices/controller.py b/esphome_device_builder/controllers/devices/controller.py index a1821d5dce..5fb8d80ee1 100644 --- a/esphome_device_builder/controllers/devices/controller.py +++ b/esphome_device_builder/controllers/devices/controller.py @@ -1329,13 +1329,9 @@ def _schedule_version_reprobe(self, configuration: str) -> None: firmware_sync.schedule_version_reprobe(self, configuration) def _deployed_name(self, device: Device) -> str: - """ - Return the hostname *device*'s firmware still answers to, if any. - - Empty while another config owns that name: whatever answers to - it is then somebody else's device, not ours. - """ - return "" if self._devices_by_name(device.deployed_name) else device.deployed_name + """Return *device*'s recorded hostname, or ``""`` while another config owns it.""" + deployed = device.deployed_name + return "" if not deployed or self._devices_by_name(deployed) else deployed def _cancel_reprobe_timers(self) -> None: """Cancel any pending post-flash re-probe timers.""" diff --git a/esphome_device_builder/controllers/devices/helpers.py b/esphome_device_builder/controllers/devices/helpers.py index 2ae67350d8..07f73f5835 100644 --- a/esphome_device_builder/controllers/devices/helpers.py +++ b/esphome_device_builder/controllers/devices/helpers.py @@ -504,13 +504,11 @@ def _build_address_cache_args( if is_local else monitor.state.dns_cache.get_cached_addresses(address) ) + if not cached and is_local and deployed_name: + # Publish the pre-rename name's IPs under the key the CLI looks up. + cached = monitor.mdns.get_cached_addresses(default_mdns_address(deployed_name)) if cached: addresses = list(cached) - if not addresses and is_local and deployed_name: - # Publish the pre-rename name's IPs under the key the CLI looks up. - stale = monitor.mdns.get_cached_addresses(default_mdns_address(deployed_name)) - if stale: - addresses = list(stale) if not addresses and device.ip: addresses = [device.ip] diff --git a/esphome_device_builder/controllers/devices/metadata.py b/esphome_device_builder/controllers/devices/metadata.py index 52337deb6a..c4ca488810 100644 --- a/esphome_device_builder/controllers/devices/metadata.py +++ b/esphome_device_builder/controllers/devices/metadata.py @@ -126,6 +126,7 @@ def _resolve_device_metadata( deployed_config_hash = str(store_md.get("deployed_config_hash", "")) deployed_version = str(store_md.get("deployed_version", "")) queued_update = bool(store_md.get("queued_update", False)) + deployed_name = str(store_md.get("deployed_name", "")) raw_api_encryption = store_md.get("api_encryption_active") api_encryption_active = raw_api_encryption if isinstance(raw_api_encryption, str) else None return DeviceFileMetadata( @@ -139,7 +140,7 @@ def _resolve_device_metadata( deployed_version=deployed_version, queued_update=queued_update, api_encryption_active=api_encryption_active, - deployed_name=str(store_md.get("deployed_name", "")), + deployed_name=deployed_name, ) def _make_metadata_resolver(self) -> MetadataResolver: @@ -225,7 +226,7 @@ def _stamp_deployed_name(self, configuration: str, *, old_name: str, new_name: s A chained rename keeps the existing record; renaming back to it clears. """ - current = self._metadata_store.get_field(configuration, "deployed_name") or old_name + current = str(self._metadata_store.get_field(configuration, "deployed_name") or old_name) deployed = "" if current == new_name else current self._metadata_store.update(configuration, deployed_name=deployed) return deployed diff --git a/esphome_device_builder/controllers/devices/state_callbacks.py b/esphome_device_builder/controllers/devices/state_callbacks.py index 78f540c095..38315161f2 100644 --- a/esphome_device_builder/controllers/devices/state_callbacks.py +++ b/esphome_device_builder/controllers/devices/state_callbacks.py @@ -82,14 +82,13 @@ def on_state_change( def on_source_change(controller: DevicesController, name: str, source: ReachabilitySource) -> None: """Update ``active_source`` and fire DEVICE_UPDATED; also clears ``deployed_name``.""" devices = controller._devices_by_name(name) - # An announce under the device's own name proves the firmware carries - # it. Above the dedupe, since a same-path rename carries the pre-rename - # ``active_source`` forward; and only when the name maps to one config, - # since siblings sharing a name share the broadcast that proves it. - heals_identity = source is ReachabilitySource.MDNS and len(devices) == 1 + # An announce under the device's own name proves the firmware carries it; + # siblings sharing a name share the broadcast, so it proves nothing there. + # Ahead of the loop's dedupe: a same-path rename carries ``active_source`` + # forward, so the row can already read mdns. + if source is ReachabilitySource.MDNS and len(devices) == 1: + controller._clear_deployed_name(devices[0].configuration) for device in devices: - if heals_identity: - controller._clear_deployed_name(device.configuration) if device.runtime_state.active_source == source: continue device.runtime_state.active_source = source diff --git a/esphome_device_builder/models/devices.py b/esphome_device_builder/models/devices.py index b87197f381..d1a64fd467 100644 --- a/esphome_device_builder/models/devices.py +++ b/esphome_device_builder/models/devices.py @@ -177,7 +177,7 @@ class Device(DashboardModel): # Hostname the running firmware still answers to after a rename that # didn't flash; empty once the YAML's own name is deployed. Backs the # OTA address cache and counts as identity for the api reviver. - deployed_name: str = "" + deployed_name: str = field(default="", metadata={"serialize": "omit"}) web_port: int | None = None current_version: str = "" # 8-char hex hash of the YAML as last successfully compiled. diff --git a/tests/controllers/firmware/test_address_cache.py b/tests/controllers/firmware/test_address_cache.py index ecfbffcabb..c9d0c333da 100644 --- a/tests/controllers/firmware/test_address_cache.py +++ b/tests/controllers/firmware/test_address_cache.py @@ -204,11 +204,10 @@ def _devices_controller_with( """Build a thin DevicesController shell with a stubbed scanner + monitor. ``get_address_cache_args`` reads the scanner's configuration-keyed - lookup, the state monitor's cached-addresses lookup, the metadata - store's ``deployed_name`` record, and the device's - ``loaded_integrations`` field — keep the rest of the controller out - of the test surface. ``names_taken`` seeds the name index the - reclaimed-record guard consults. + lookup, the state monitor's cached-addresses lookup, and the device's + ``deployed_name`` / ``loaded_integrations`` fields — keep the rest of + the controller out of the test surface. ``names_taken`` seeds the name + index the reclaimed-record guard consults. """ controller = DevicesController.__new__(DevicesController) scanner = RecordingScanner(devices_by_name={name: [object()] for name in names_taken}) @@ -312,7 +311,7 @@ def test_get_address_cache_args_drops_a_reclaimed_deployed_name() -> None: def test_get_address_cache_args_reads_the_deployed_name_record() -> None: - """The controller feeds the store's record into the cache-args build.""" + """The controller feeds the device's record into the cache-args build.""" controller = _devices_controller_with( _device(loaded_integrations=["api"], deployed_name="asistente"), monitor=RecordingStateMonitor(cached_addresses={"asistente.local": ["192.168.1.50"]}), diff --git a/tests/test_api_reviver.py b/tests/test_api_reviver.py index bf45ed8fd3..ebf671c35b 100644 --- a/tests/test_api_reviver.py +++ b/tests/test_api_reviver.py @@ -35,6 +35,7 @@ from .conftest import ( RecordingMonitorCallbacks, + make_device, make_state_monitor_with_callbacks, make_stuck_offline_device, ) @@ -367,6 +368,19 @@ async def test_recorded_pre_rename_name_revives_instead_of_invalidating() -> Non assert ("on_state_change", "kitchen", DeviceState.ONLINE, "ping") in callbacks.calls +async def test_a_reclaimed_pre_rename_name_is_not_identity() -> None: + """Another config owns that name now, so the answer isn't ours to read.""" + device = make_stuck_offline_device(deployed_name="asistente") + sibling = make_device("asistente") + _monitor, callbacks, src = _reviver( + [device, sibling], worker_result={**_WORKER_MATCH, "name": "asistente"} + ) + + await src._sweep() + + assert ("on_persisted_ip_invalidated", "kitchen", "192.168.1.50") in callbacks.calls + + async def test_name_mismatch_invalidates_the_persisted_ip() -> None: device = make_stuck_offline_device() monitor, callbacks, src = _reviver( diff --git a/tests/test_device_state_event.py b/tests/test_device_state_event.py index a992b1e181..22b732c137 100644 --- a/tests/test_device_state_event.py +++ b/tests/test_device_state_event.py @@ -10,6 +10,10 @@ from __future__ import annotations +from typing import Any + +import pytest + from esphome_device_builder.models import DeviceState, EventType, ReachabilitySource from .conftest import make_device, make_devices_controller_with_bus @@ -60,15 +64,41 @@ def test_state_change_unknown_device_does_not_fire() -> None: assert captured == [] -async def test_mdns_ownership_clears_the_deployed_name() -> None: - """An announce under the YAML's name proves the firmware carries it (#2730).""" - device = make_device(address="") - ctrl, _ = make_devices_controller_with_bus([device]) +@pytest.mark.parametrize( + ("devices", "source", "expected"), + [ + pytest.param([{}], ReachabilitySource.MDNS, None, id="mdns_clears"), + # Ping reaches the device through the record itself; it proves no name. + pytest.param([{}], ReachabilitySource.PING, "asistente", id="ping_keeps"), + # A same-path rename carries ``active_source`` forward as mdns. + pytest.param( + [{"active_source": ReachabilitySource.MDNS}], + ReachabilitySource.MDNS, + None, + id="stale_active_source", + ), + # Siblings share one broadcast, so it can't prove which was flashed. + pytest.param( + [{}, {"configuration": "kitchen (1).yaml"}], + ReachabilitySource.MDNS, + "asistente", + id="shared_name_keeps", + ), + ], +) +async def test_mdns_ownership_clears_the_deployed_name( + devices: list[dict[str, Any]], + source: ReachabilitySource, + expected: str | None, +) -> None: + """Only an mDNS announce that identifies one config proves the deployed name (#2730).""" + rows = [make_device(address="", **kwargs) for kwargs in devices] + ctrl, _ = make_devices_controller_with_bus(rows) ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) - ctrl._on_source_change("kitchen", ReachabilitySource.MDNS) + ctrl._on_source_change("kitchen", source) - assert "deployed_name" not in ctrl._metadata_store.get("kitchen.yaml") + assert ctrl._metadata_store.get("kitchen.yaml").get("deployed_name") == expected async def test_mdns_ownership_of_a_ping_online_device_still_clears() -> None: @@ -81,37 +111,3 @@ async def test_mdns_ownership_of_a_ping_online_device_still_clears() -> None: ctrl._on_source_change("kitchen", ReachabilitySource.MDNS) assert "deployed_name" not in ctrl._metadata_store.get("kitchen.yaml") - - -async def test_ping_ownership_keeps_the_deployed_name() -> None: - """Ping reaches the device through the record itself; it proves no name.""" - device = make_device(address="") - ctrl, _ = make_devices_controller_with_bus([device]) - ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) - - ctrl._on_source_change("kitchen", ReachabilitySource.PING) - - assert ctrl._metadata_store.get("kitchen.yaml")["deployed_name"] == "asistente" - - -async def test_mdns_ownership_clears_through_a_stale_active_source() -> None: - """A same-path rename carries ``active_source`` forward as mdns (#2730).""" - device = make_device(address="", active_source=ReachabilitySource.MDNS) - ctrl, _ = make_devices_controller_with_bus([device]) - ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) - - ctrl._on_source_change("kitchen", ReachabilitySource.MDNS) - - assert "deployed_name" not in ctrl._metadata_store.get("kitchen.yaml") - - -async def test_mdns_of_a_shared_name_keeps_both_deployed_names() -> None: - """Siblings share one broadcast, so it can't prove which one was flashed.""" - first = make_device(configuration="kitchen.yaml", address="") - second = make_device(configuration="kitchen (1).yaml", address="") - ctrl, _ = make_devices_controller_with_bus([first, second]) - ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) - - ctrl._on_source_change("kitchen", ReachabilitySource.MDNS) - - assert ctrl._metadata_store.get("kitchen.yaml")["deployed_name"] == "asistente" From 9237ef59529edcb934efd60c17e69133611c8795 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 22 Sep 2026 08:18:23 +0100 Subject: [PATCH 12/15] Write the record to the live row, not just the store Both readers consult Device.deployed_name, but the mDNS self-heal and the scan-change stamp never trigger a reload, so the row kept a stale record for the process lifetime while the store moved on. The accessors now land on the controller and write the indexed row and the store together, the on_ip_change shape. The in-place rename never rode the migrate's flush (store.rename returns early when the names match), so the stamp is no longer debounced either. Drop the orphan test: a clear on a missing entry is a no-op by construction, so it could not fail. Add an e2e for the reporter's loop: config-only rename, then install, with the zeroconf cache answering the old name at a different address than the persisted IP, so the upload argv proves the redirect rather than the fallback. --- .../controllers/devices/controller.py | 24 ++++ .../controllers/devices/metadata.py | 16 --- .../controllers/devices/mutations_simple.py | 2 +- tests/conftest.py | 3 + .../devices/test_branches_coverage.py | 3 + .../devices/test_rename_config_only.py | 18 --- tests/controllers/firmware/test_refresh.py | 15 ++ tests/e2e/test_rename_keeps_ota_reachable.py | 130 ++++++++++++++++++ tests/test_device_state_event.py | 4 +- 9 files changed, 179 insertions(+), 36 deletions(-) create mode 100644 tests/e2e/test_rename_keeps_ota_reachable.py diff --git a/esphome_device_builder/controllers/devices/controller.py b/esphome_device_builder/controllers/devices/controller.py index 5fb8d80ee1..df94466d4a 100644 --- a/esphome_device_builder/controllers/devices/controller.py +++ b/esphome_device_builder/controllers/devices/controller.py @@ -1333,6 +1333,30 @@ def _deployed_name(self, device: Device) -> str: deployed = device.deployed_name return "" if not deployed or self._devices_by_name(deployed) else deployed + def _stamp_deployed_name(self, configuration: str, *, old_name: str, new_name: str) -> str: + """ + Record the hostname the firmware answers to; returns it (``""`` if cleared). + + A chained rename keeps the existing record; renaming back to it clears. + """ + current = str(self._metadata_store.get_field(configuration, "deployed_name") or old_name) + deployed = "" if current == new_name else current + self._set_deployed_name(configuration, deployed) + return deployed + + def _clear_deployed_name(self, configuration: str) -> None: + """Forget the recorded hostname; the firmware carries the YAML's own name.""" + if self._metadata_store.get_field(configuration, "deployed_name"): + self._set_deployed_name(configuration, "") + + def _set_deployed_name(self, configuration: str, deployed: str) -> None: + """Write the record to the live row and the store; readers consult the row.""" + device = self._scanner.get_by_configuration(configuration) + if device is not None: + device.deployed_name = deployed + # Renames are rare and the YAML is already durable; don't debounce the record. + self._metadata_store.update(configuration, deployed_name=deployed, delay=0.0) + def _cancel_reprobe_timers(self) -> None: """Cancel any pending post-flash re-probe timers.""" for handle in self._reprobe_timers.values(): diff --git a/esphome_device_builder/controllers/devices/metadata.py b/esphome_device_builder/controllers/devices/metadata.py index c4ca488810..72092b9a98 100644 --- a/esphome_device_builder/controllers/devices/metadata.py +++ b/esphome_device_builder/controllers/devices/metadata.py @@ -220,22 +220,6 @@ async def _migrate_device_metadata( await self._metadata_store.rename(old_configuration, new_configuration) await self._shared_sidecar.rename(old_configuration, new_configuration) - def _stamp_deployed_name(self, configuration: str, *, old_name: str, new_name: str) -> str: - """ - Record the hostname the firmware answers to; returns it (``""`` if cleared). - - A chained rename keeps the existing record; renaming back to it clears. - """ - current = str(self._metadata_store.get_field(configuration, "deployed_name") or old_name) - deployed = "" if current == new_name else current - self._metadata_store.update(configuration, deployed_name=deployed) - return deployed - - def _clear_deployed_name(self, configuration: str) -> None: - """Forget the recorded hostname; the firmware carries the YAML's own name.""" - if self._metadata_store.get_field(configuration, "deployed_name"): - self._metadata_store.update(configuration, deployed_name="") - async def _clear_volatile_device_metadata(self, configuration: str) -> None: """Clear archive-volatile fields in both stores (keeps identity). diff --git a/esphome_device_builder/controllers/devices/mutations_simple.py b/esphome_device_builder/controllers/devices/mutations_simple.py index 0f469b13fb..9d949e5dad 100644 --- a/esphome_device_builder/controllers/devices/mutations_simple.py +++ b/esphome_device_builder/controllers/devices/mutations_simple.py @@ -339,7 +339,7 @@ def _land() -> None: for name in sorted({os.path.normpath(n) for n in (configuration, new_filename)}): await locks.enter_async_context(controller._yaml_write_lock(name)) await run_in_executor(_land) - # Before the migrate, so its immediate flush carries the record. + # Before the migrate, so a moved file carries the record with it. deployed_name = controller._stamp_deployed_name( configuration, old_name=old_name, new_name=new_name ) diff --git a/tests/conftest.py b/tests/conftest.py index b502c0d59f..7079311cf4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1013,6 +1013,9 @@ def make_devices_controller_with_bus( for device in devices: by_name.setdefault(device.name, []).append(device) controller._scanner.get_by_name = lambda name: by_name.get(name, []) + controller._scanner.get_by_configuration = lambda configuration: next( + (d for d in devices if d.configuration == configuration), None + ) # Real metadata stores anchored at a TemporaryDirectory whose # lifetime is pinned to the controller; ``__del__`` cleans up # the dir when the test releases its reference. diff --git a/tests/controllers/devices/test_branches_coverage.py b/tests/controllers/devices/test_branches_coverage.py index efd3dc2c16..120e0b932d 100644 --- a/tests/controllers/devices/test_branches_coverage.py +++ b/tests/controllers/devices/test_branches_coverage.py @@ -1662,10 +1662,13 @@ async def test_on_scan_change_name_change_records_the_deployed_name( controller = make_controller(tmp_path, with_state_monitor=True) compiled = _device("livingroom", loaded_integrations=["api"]) + # The scanner indexes the row before it notifies; the stamp must reach it. + controller._scanner.devices = [compiled] controller._on_scan_change(ScanChange.UPDATED, compiled, _device("kitchen")) assert controller._metadata_store.get(compiled.configuration)["deployed_name"] == "kitchen" + assert compiled.deployed_name == "kitchen" async def test_on_scan_change_unbuilt_name_change_records_nothing( diff --git a/tests/controllers/devices/test_rename_config_only.py b/tests/controllers/devices/test_rename_config_only.py index 76247a7517..541dcfd413 100644 --- a/tests/controllers/devices/test_rename_config_only.py +++ b/tests/controllers/devices/test_rename_config_only.py @@ -521,21 +521,3 @@ async def _rescan_fires_scan_change(_controller: object, configuration: str) -> ) assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") - - -async def test_rename_leaves_no_orphan_when_the_entry_moved_away( - tmp_path: Path, make_controller: MakeControllerFactory -) -> None: - """A concurrent rename migrating the entry must not get it re-created (#2730).""" - controller = make_controller(tmp_path) - (tmp_path / "kitchen.yaml").write_text(_YAML, encoding="utf-8") - - async def _rescan_steals_the_entry(_controller: object, configuration: str) -> None: - await controller._metadata_store.remove(configuration) - - with patch.object(mutations_simple, "rescan_renamed", _rescan_steals_the_entry): - await controller.rename_device( - configuration="kitchen.yaml", new_name="livingroom", config_only=True - ) - - assert controller._metadata_store.get("livingroom.yaml") == {} diff --git a/tests/controllers/firmware/test_refresh.py b/tests/controllers/firmware/test_refresh.py index 37297f4391..ae4b05017f 100644 --- a/tests/controllers/firmware/test_refresh.py +++ b/tests/controllers/firmware/test_refresh.py @@ -182,6 +182,21 @@ async def test_completed_app_upload_clears_the_deployed_name(tmp_path: Path) -> assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") +async def test_completed_app_upload_clears_the_live_row_too(tmp_path: Path) -> None: + """Both readers consult the row, and the clear lands before the background reload.""" + controller, _ = _make_controller(tmp_path) + row = make_device(deployed_name="asistente") + controller._scanner = RecordingScanner() + controller._scanner.devices = [row] + controller._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + + controller._on_firmware_job_completed( + Event(EventType.JOB_COMPLETED, {"job": _job(JobType.UPLOAD, JobStatus.COMPLETED)}) + ) + + assert row.deployed_name == "" + + async def test_completed_bootloader_upload_keeps_the_deployed_name(tmp_path: Path) -> None: """``--bootloader`` replaces no app, so the recorded hostname still stands.""" controller, _ = _make_controller(tmp_path) diff --git a/tests/e2e/test_rename_keeps_ota_reachable.py b/tests/e2e/test_rename_keeps_ota_reachable.py new file mode 100644 index 0000000000..200741943d --- /dev/null +++ b/tests/e2e/test_rename_keeps_ota_reachable.py @@ -0,0 +1,130 @@ +"""E2E: a config-only rename leaves the firmware on its old hostname; the install still lands.""" + +from __future__ import annotations + +import asyncio +import json +import socket +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock + +import pytest +from zeroconf import DNSAddress, Zeroconf, current_time_millis +from zeroconf.const import _CLASS_IN, _TYPE_A + +from esphome_device_builder.device_builder import DeviceBuilder +from esphome_device_builder.models import DeviceState, EventType, JobStatus, JobType + +from ..conftest import MakeSettingsFactory, record_argv_esphome + +# ``api:`` so the row loads an OTA-capable integration and gets cache args at all. +_YAML = """\ +esphome: + name: kitchen + +esp8266: + board: esp01_1m + +wifi: + ssid: lan + password: password1 + +api: +""" + + +@pytest.fixture +async def dashboard( + make_settings: MakeSettingsFactory, + _hermetic_lifecycle: None, + tmp_path: Path, +) -> Any: + """Real ``DeviceBuilder`` with one device tracked from the initial scan.""" + (tmp_path / "kitchen.yaml").write_text(_YAML, encoding="utf-8") + settings = make_settings(with_core_path=True) + settings.using_password = False + db = DeviceBuilder(settings) + await db.start() + try: + yield db + finally: + await db.stop() + + +def _completed_job(db: DeviceBuilder, job_type: JobType) -> asyncio.Future: + """Future resolved by the JOB_COMPLETED bus event for the next *job_type* job.""" + done: asyncio.Future = asyncio.get_running_loop().create_future() + + def _on_completed(event: Any) -> None: + job = event.data["job"] + if job.job_type is job_type and job.status is JobStatus.COMPLETED and not done.done(): + done.set_result(job) + + unsub = db.bus.add_listener(EventType.JOB_COMPLETED, _on_completed) + done.add_done_callback(lambda _f: unsub()) + return done + + +def _cache_a_record(zc: Zeroconf, host: str, ip: str) -> None: + """Seed *zc*'s cache as a live announce of *host* at *ip* would.""" + record = DNSAddress( + name=host, + type_=_TYPE_A, + class_=_CLASS_IN, + ttl=120, + address=socket.inet_aton(ip), + created=current_time_millis(), + ) + zc.cache.async_add_records([record]) + + +async def test_config_only_rename_then_install_targets_the_old_hostname( + dashboard: DeviceBuilder, tmp_path: Path +) -> None: + """The reporter's loop: rename without a flash, then install (#2730).""" + db = dashboard + assert db.firmware is not None + assert db.devices is not None + monitor = db.devices._state_monitor + argv_log = tmp_path / "argv.jsonl" + record_argv_esphome(db.firmware.state, argv_log) + + # The firmware answered ``kitchen`` at .50 before the rename. + monitor.apply("kitchen", DeviceState.ONLINE, "mdns", claim=True) + monitor.apply_ip("kitchen", "192.168.1.50") + + result = await db.devices.rename_device( + configuration="kitchen.yaml", new_name="livingroom", config_only=True + ) + + assert result == {"configuration": "livingroom.yaml", "job": None} + device = db.devices.get_by_configuration("livingroom.yaml") + assert device is not None + # The live row is what the address-cache build and the reviver read. + assert device.deployed_name == "kitchen" + + # A DHCP move since: what answers ``kitchen.local`` now is .60, not the + # persisted .50, and it's the old name the firmware still broadcasts. + zc = Zeroconf(interfaces=["127.0.0.1"]) + try: + _cache_a_record(zc, "kitchen.local.", "192.168.1.60") + monitor.mdns._zeroconf = MagicMock(zeroconf=zc) + + upload_done = _completed_job(db, JobType.UPLOAD) + await db.firmware.install(configuration="livingroom.yaml", port="OTA") + await asyncio.wait_for(upload_done, timeout=5.0) + finally: + zc.close() + + invocations = [json.loads(line) for line in argv_log.read_text().splitlines()] + upload = next(argv for argv in invocations if "upload" in argv) + cache_arg = upload[upload.index("--mdns-address-cache") + 1] + assert cache_arg == "livingroom.local=192.168.1.60" + # The landed flash retires the record, on the indexed row and on disk. The + # compile's post-job reload rebuilt the row, so read it back the way the + # readers do rather than through the pre-install handle. + device = db.devices.get_by_configuration("livingroom.yaml") + assert device is not None + assert device.deployed_name == "" + assert "deployed_name" not in db.devices._metadata_store.get("livingroom.yaml") diff --git a/tests/test_device_state_event.py b/tests/test_device_state_event.py index 22b732c137..3e178ceba4 100644 --- a/tests/test_device_state_event.py +++ b/tests/test_device_state_event.py @@ -92,13 +92,15 @@ async def test_mdns_ownership_clears_the_deployed_name( expected: str | None, ) -> None: """Only an mDNS announce that identifies one config proves the deployed name (#2730).""" - rows = [make_device(address="", **kwargs) for kwargs in devices] + rows = [make_device(address="", deployed_name="asistente", **kwargs) for kwargs in devices] ctrl, _ = make_devices_controller_with_bus(rows) ctrl._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) ctrl._on_source_change("kitchen", source) assert ctrl._metadata_store.get("kitchen.yaml").get("deployed_name") == expected + # Both readers consult the row, and no reload follows this clear. + assert rows[0].deployed_name == (expected or "") async def test_mdns_ownership_of_a_ping_online_device_still_clears() -> None: From ab6a0a42245178d3bdff64010389fb6071f6f5b4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 22 Sep 2026 08:28:09 +0100 Subject: [PATCH 13/15] Hoist the e2e bring-up and tighten the record writers The single-device DeviceBuilder bring-up and the JOB_COMPLETED waiter were copied between two e2e modules; both live in the e2e conftest now. The bus-controller factory hand-rolled two scanner lambdas that RecordingScanner already provides. Only the stamp is written immediately: a lost stamp strands the device, a lost clear self-heals on the next announce. Callers that already hold the row pass it instead of re-looking it up, and the two flash-clear tests fold into one that asserts the row and the store. --- .../_device_state_monitor/api_reviver.py | 4 +-- .../controllers/devices/controller.py | 26 ++++++++------ .../controllers/devices/scan_change.py | 2 +- .../controllers/devices/state_callbacks.py | 8 ++--- tests/conftest.py | 18 ++++------ tests/controllers/firmware/test_refresh.py | 16 ++------- tests/e2e/conftest.py | 34 +++++++++++++++++++ tests/e2e/test_offline_queue_wake.py | 34 +++---------------- tests/e2e/test_rename_keeps_ota_reachable.py | 34 +++---------------- 9 files changed, 75 insertions(+), 101 deletions(-) diff --git a/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py b/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py index 638d56adc4..2e9a4493fc 100644 --- a/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py +++ b/esphome_device_builder/controllers/_device_state_monitor/api_reviver.py @@ -217,9 +217,7 @@ async def _verify_and_revive(self, device: Device, ip: str, rtt: float) -> None: ) self._record_dial_failure(key) return - # A device renamed without a flash still answers its recorded - # pre-rename name, so that name identifies it too — unless another - # config has since claimed it, when the answer isn't ours to read. + # The recorded pre-rename name is identity too, unless another config owns it. recorded = device.deployed_name if recorded and monitor._get_devices_by_name(recorded): recorded = "" diff --git a/esphome_device_builder/controllers/devices/controller.py b/esphome_device_builder/controllers/devices/controller.py index df94466d4a..5056fa9913 100644 --- a/esphome_device_builder/controllers/devices/controller.py +++ b/esphome_device_builder/controllers/devices/controller.py @@ -84,7 +84,7 @@ ) from ._ignored_devices_store import SAVE_DELAY as _IGNORED_DEVICES_SAVE_DELAY from ._ignored_devices_store import ignored_devices_store -from ._metadata_store import DeviceMetadataStore +from ._metadata_store import _DEFAULT_SAVE_DELAY, DeviceMetadataStore from ._pending_keys_store import PendingKeysStore from ._shared_sidecar import SharedSidecarClient from ._state import DevicesState @@ -1333,7 +1333,9 @@ def _deployed_name(self, device: Device) -> str: deployed = device.deployed_name return "" if not deployed or self._devices_by_name(deployed) else deployed - def _stamp_deployed_name(self, configuration: str, *, old_name: str, new_name: str) -> str: + def _stamp_deployed_name( + self, configuration: str, *, old_name: str, new_name: str, device: Device | None = None + ) -> str: """ Record the hostname the firmware answers to; returns it (``""`` if cleared). @@ -1341,21 +1343,25 @@ def _stamp_deployed_name(self, configuration: str, *, old_name: str, new_name: s """ current = str(self._metadata_store.get_field(configuration, "deployed_name") or old_name) deployed = "" if current == new_name else current - self._set_deployed_name(configuration, deployed) + self._set_deployed_name(configuration, deployed, device=device) return deployed - def _clear_deployed_name(self, configuration: str) -> None: + def _clear_deployed_name(self, configuration: str, *, device: Device | None = None) -> None: """Forget the recorded hostname; the firmware carries the YAML's own name.""" if self._metadata_store.get_field(configuration, "deployed_name"): - self._set_deployed_name(configuration, "") + self._set_deployed_name(configuration, "", device=device) - def _set_deployed_name(self, configuration: str, deployed: str) -> None: - """Write the record to the live row and the store; readers consult the row.""" - device = self._scanner.get_by_configuration(configuration) + def _set_deployed_name( + self, configuration: str, deployed: str, *, device: Device | None = None + ) -> None: + """Write the record to the live row (*device*, or the indexed one) and the store.""" + if device is None: + device = self._scanner.get_by_configuration(configuration) if device is not None: device.deployed_name = deployed - # Renames are rare and the YAML is already durable; don't debounce the record. - self._metadata_store.update(configuration, deployed_name=deployed, delay=0.0) + # A lost stamp strands the device; a lost clear self-heals on the next announce. + delay = 0.0 if deployed else _DEFAULT_SAVE_DELAY + self._metadata_store.update(configuration, deployed_name=deployed, delay=delay) def _cancel_reprobe_timers(self) -> None: """Cancel any pending post-flash re-probe timers.""" diff --git a/esphome_device_builder/controllers/devices/scan_change.py b/esphome_device_builder/controllers/devices/scan_change.py index 6cd4da94e8..a092ba2407 100644 --- a/esphome_device_builder/controllers/devices/scan_change.py +++ b/esphome_device_builder/controllers/devices/scan_change.py @@ -164,7 +164,7 @@ def _reconcile_rename( # filename-stem placeholder no firmware answered to. if device.loaded_integrations: controller._stamp_deployed_name( - device.configuration, old_name=previous.name, new_name=device.name + device.configuration, old_name=previous.name, new_name=device.name, device=device ) # The ADDED retraction keys on the name at add time; the freed old # name may have a suppressed announcement worth resurfacing. diff --git a/esphome_device_builder/controllers/devices/state_callbacks.py b/esphome_device_builder/controllers/devices/state_callbacks.py index 38315161f2..e6ebba9cc2 100644 --- a/esphome_device_builder/controllers/devices/state_callbacks.py +++ b/esphome_device_builder/controllers/devices/state_callbacks.py @@ -82,12 +82,10 @@ def on_state_change( def on_source_change(controller: DevicesController, name: str, source: ReachabilitySource) -> None: """Update ``active_source`` and fire DEVICE_UPDATED; also clears ``deployed_name``.""" devices = controller._devices_by_name(name) - # An announce under the device's own name proves the firmware carries it; - # siblings sharing a name share the broadcast, so it proves nothing there. - # Ahead of the loop's dedupe: a same-path rename carries ``active_source`` - # forward, so the row can already read mdns. + # Only an announce that maps to one config proves the firmware carries the name; + # above the dedupe since a same-path rename carries ``active_source`` forward. if source is ReachabilitySource.MDNS and len(devices) == 1: - controller._clear_deployed_name(devices[0].configuration) + controller._clear_deployed_name(devices[0].configuration, device=devices[0]) for device in devices: if device.runtime_state.active_source == source: continue diff --git a/tests/conftest.py b/tests/conftest.py index 7079311cf4..8aaff872b5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -77,6 +77,7 @@ ReachabilitySource, ) from tests._mqtt_fixtures import RecordingMonitor +from tests._recording_scanner import RecordingScanner if TYPE_CHECKING: from blockbuster import BlockBuster @@ -987,10 +988,9 @@ def make_devices_controller_with_bus( only care about a subset filter the list themselves (``[e for e in captured if e.event_type == X]``). - The scanner is a ``MagicMock`` exposing ``devices`` and a - ``get_by_name(name)`` lambda derived from *devices*; mirrors - the production ``DeviceScanner``'s name-keyed grouping closely - enough for the callback paths these tests exercise. + The scanner is a ``RecordingScanner`` seeded with *devices* and + their name-keyed grouping, so ``get_by_name`` / + ``get_by_configuration`` answer like production. ``create_background_task`` lets callers wire a side-effect function (e.g. closing the coroutine to avoid @@ -1007,15 +1007,11 @@ def make_devices_controller_with_bus( if create_background_task is not None: controller._db.create_background_task = MagicMock(side_effect=create_background_task) controller._db.bus = bus - controller._scanner = MagicMock() - controller._scanner.devices = devices - by_name: dict[str, list[Device]] = {} + by_name: dict[str, list[object]] = {} for device in devices: by_name.setdefault(device.name, []).append(device) - controller._scanner.get_by_name = lambda name: by_name.get(name, []) - controller._scanner.get_by_configuration = lambda configuration: next( - (d for d in devices if d.configuration == configuration), None - ) + controller._scanner = RecordingScanner(devices_by_name=by_name) + controller._scanner.devices = list(devices) # Real metadata stores anchored at a TemporaryDirectory whose # lifetime is pinned to the controller; ``__del__`` cleans up # the dir when the test releases its reference. diff --git a/tests/controllers/firmware/test_refresh.py b/tests/controllers/firmware/test_refresh.py index ae4b05017f..3578c80655 100644 --- a/tests/controllers/firmware/test_refresh.py +++ b/tests/controllers/firmware/test_refresh.py @@ -171,19 +171,7 @@ def test_completed_install_recomputes_hash_and_reloads(tmp_path: Path) -> None: async def test_completed_app_upload_clears_the_deployed_name(tmp_path: Path) -> None: - """The app image now carries the YAML's own name (#2730).""" - controller, _ = _make_controller(tmp_path) - controller._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) - - controller._on_firmware_job_completed( - Event(EventType.JOB_COMPLETED, {"job": _job(JobType.UPLOAD, JobStatus.COMPLETED)}) - ) - - assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") - - -async def test_completed_app_upload_clears_the_live_row_too(tmp_path: Path) -> None: - """Both readers consult the row, and the clear lands before the background reload.""" + """The app image now carries the YAML's own name; row and store both clear (#2730).""" controller, _ = _make_controller(tmp_path) row = make_device(deployed_name="asistente") controller._scanner = RecordingScanner() @@ -194,6 +182,8 @@ async def test_completed_app_upload_clears_the_live_row_too(tmp_path: Path) -> N Event(EventType.JOB_COMPLETED, {"job": _job(JobType.UPLOAD, JobStatus.COMPLETED)}) ) + assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") + # Both readers consult the row, and the clear lands before the background reload. assert row.deployed_name == "" diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 746ab93bf3..8b7cec02e6 100644 --- a/tests/e2e/conftest.py +++ b/tests/e2e/conftest.py @@ -61,6 +61,7 @@ PEER_LINK_PATH, make_peer_link_handler, ) +from esphome_device_builder.device_builder import DeviceBuilder from esphome_device_builder.helpers.event_bus import EventBus from esphome_device_builder.helpers.peer_link_identity import PeerLinkIdentityStore from esphome_device_builder.helpers.remote_artifacts_materialise import ( @@ -86,6 +87,39 @@ ) +@asynccontextmanager +async def single_device_dashboard( + make_settings: Any, tmp_path: Path, yaml: str, filename: str = "kitchen.yaml" +) -> AsyncIterator[DeviceBuilder]: + """Real ``DeviceBuilder`` with one device tracked from the initial scan.""" + (tmp_path / filename).write_text(yaml, encoding="utf-8") + settings = make_settings(with_core_path=True) + settings.using_password = False + db = DeviceBuilder(settings) + await db.start() + try: + yield db + finally: + await db.stop() + + +def completed_job(db: DeviceBuilder, job_type: JobType) -> asyncio.Future: + """Future resolved by the JOB_COMPLETED bus event for the next *job_type* job. + + Registered before the trigger so the completion can't be missed. + """ + done: asyncio.Future = asyncio.get_running_loop().create_future() + + def _on_completed(event: Any) -> None: + job = event.data["job"] + if job.job_type is job_type and job.status is JobStatus.COMPLETED and not done.done(): + done.set_result(job) + + unsub = db.bus.add_listener(EventType.JOB_COMPLETED, _on_completed) + done.add_done_callback(lambda _f: unsub()) + return done + + @dataclass class PairedInstances: """Two paired dashboards + a TestServer, pre-paired and ready to drive. diff --git a/tests/e2e/test_offline_queue_wake.py b/tests/e2e/test_offline_queue_wake.py index 89d737d906..047948931f 100644 --- a/tests/e2e/test_offline_queue_wake.py +++ b/tests/e2e/test_offline_queue_wake.py @@ -10,9 +10,10 @@ import pytest from esphome_device_builder.device_builder import DeviceBuilder -from esphome_device_builder.models import DeviceState, EventType, JobStatus, JobType +from esphome_device_builder.models import DeviceState, EventType, JobType from ..conftest import MakeSettingsFactory, record_argv_esphome +from .conftest import completed_job, single_device_dashboard _ESP8266_YAML = "esphome:\n name: kitchen\n\nesp8266:\n board: esp01_1m\n" @@ -23,16 +24,8 @@ async def dashboard( _hermetic_lifecycle: None, tmp_path: Path, ) -> Any: - """Real ``DeviceBuilder`` with one esp8266 device tracked from the initial scan.""" - (tmp_path / "kitchen.yaml").write_text(_ESP8266_YAML, encoding="utf-8") - settings = make_settings(with_core_path=True) - settings.using_password = False - db = DeviceBuilder(settings) - await db.start() - try: + async with single_device_dashboard(make_settings, tmp_path, _ESP8266_YAML) as db: yield db - finally: - await db.stop() def _announce(db: DeviceBuilder, state: DeviceState, source: str) -> None: @@ -41,23 +34,6 @@ def _announce(db: DeviceBuilder, state: DeviceState, source: str) -> None: db.devices._state_monitor.apply("kitchen", state, source, claim=True) -def _completed_job(db: DeviceBuilder, job_type: JobType) -> asyncio.Future: - """Future resolved by the JOB_COMPLETED bus event for the next *job_type* job. - - Registered before the trigger so the completion can't be missed. - """ - done: asyncio.Future = asyncio.get_running_loop().create_future() - - def _on_completed(event: Any) -> None: - job = event.data["job"] - if job.job_type is job_type and job.status is JobStatus.COMPLETED and not done.done(): - done.set_result(job) - - unsub = db.bus.add_listener(EventType.JOB_COMPLETED, _on_completed) - done.add_done_callback(lambda _f: unsub()) - return done - - async def _wait_queued_update(db: DeviceBuilder, *, expected: bool, timeout: float = 5.0) -> None: """Wait for ``queued_update`` == *expected*, driven by DEVICE_UPDATED events.""" assert db.devices is not None @@ -92,7 +68,7 @@ async def test_offline_install_defers_then_flashes_on_wake( record_argv_esphome(db.firmware.state, argv_log) _announce(db, DeviceState.OFFLINE, "ping") - compile_done = _completed_job(db, JobType.COMPILE) + compile_done = completed_job(db, JobType.COMPILE) job = await db.firmware.install(configuration="kitchen.yaml", port="OTA") assert job.job_type is JobType.COMPILE @@ -106,7 +82,7 @@ async def test_offline_install_defers_then_flashes_on_wake( # The fake announcement flips the device ONLINE through the same # monitor -> state-callback -> DEVICE_STATE_CHANGED path zeroconf drives. - upload_done = _completed_job(db, JobType.UPLOAD) + upload_done = completed_job(db, JobType.UPLOAD) _announce(db, DeviceState.ONLINE, "mdns") await asyncio.wait_for(upload_done, timeout=5.0) await _wait_queued_update(db, expected=False) diff --git a/tests/e2e/test_rename_keeps_ota_reachable.py b/tests/e2e/test_rename_keeps_ota_reachable.py index 200741943d..305e5b1b2a 100644 --- a/tests/e2e/test_rename_keeps_ota_reachable.py +++ b/tests/e2e/test_rename_keeps_ota_reachable.py @@ -14,9 +14,10 @@ from zeroconf.const import _CLASS_IN, _TYPE_A from esphome_device_builder.device_builder import DeviceBuilder -from esphome_device_builder.models import DeviceState, EventType, JobStatus, JobType +from esphome_device_builder.models import DeviceState, JobType from ..conftest import MakeSettingsFactory, record_argv_esphome +from .conftest import completed_job, single_device_dashboard # ``api:`` so the row loads an OTA-capable integration and gets cache args at all. _YAML = """\ @@ -40,30 +41,8 @@ async def dashboard( _hermetic_lifecycle: None, tmp_path: Path, ) -> Any: - """Real ``DeviceBuilder`` with one device tracked from the initial scan.""" - (tmp_path / "kitchen.yaml").write_text(_YAML, encoding="utf-8") - settings = make_settings(with_core_path=True) - settings.using_password = False - db = DeviceBuilder(settings) - await db.start() - try: + async with single_device_dashboard(make_settings, tmp_path, _YAML) as db: yield db - finally: - await db.stop() - - -def _completed_job(db: DeviceBuilder, job_type: JobType) -> asyncio.Future: - """Future resolved by the JOB_COMPLETED bus event for the next *job_type* job.""" - done: asyncio.Future = asyncio.get_running_loop().create_future() - - def _on_completed(event: Any) -> None: - job = event.data["job"] - if job.job_type is job_type and job.status is JobStatus.COMPLETED and not done.done(): - done.set_result(job) - - unsub = db.bus.add_listener(EventType.JOB_COMPLETED, _on_completed) - done.add_done_callback(lambda _f: unsub()) - return done def _cache_a_record(zc: Zeroconf, host: str, ip: str) -> None: @@ -101,7 +80,6 @@ async def test_config_only_rename_then_install_targets_the_old_hostname( assert result == {"configuration": "livingroom.yaml", "job": None} device = db.devices.get_by_configuration("livingroom.yaml") assert device is not None - # The live row is what the address-cache build and the reviver read. assert device.deployed_name == "kitchen" # A DHCP move since: what answers ``kitchen.local`` now is .60, not the @@ -111,7 +89,7 @@ async def test_config_only_rename_then_install_targets_the_old_hostname( _cache_a_record(zc, "kitchen.local.", "192.168.1.60") monitor.mdns._zeroconf = MagicMock(zeroconf=zc) - upload_done = _completed_job(db, JobType.UPLOAD) + upload_done = completed_job(db, JobType.UPLOAD) await db.firmware.install(configuration="livingroom.yaml", port="OTA") await asyncio.wait_for(upload_done, timeout=5.0) finally: @@ -121,9 +99,7 @@ async def test_config_only_rename_then_install_targets_the_old_hostname( upload = next(argv for argv in invocations if "upload" in argv) cache_arg = upload[upload.index("--mdns-address-cache") + 1] assert cache_arg == "livingroom.local=192.168.1.60" - # The landed flash retires the record, on the indexed row and on disk. The - # compile's post-job reload rebuilt the row, so read it back the way the - # readers do rather than through the pre-install handle. + # The compile's post-job reload rebuilt the row; re-fetch instead of reusing the handle. device = db.devices.get_by_configuration("livingroom.yaml") assert device is not None assert device.deployed_name == "" From 07dff09d2ca046d2d654abcd4249984798deb16c Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 22 Sep 2026 08:50:57 +0100 Subject: [PATCH 14/15] Keep the record's writers in the metadata mixin controller.py is already far past the 800-line cap and metadata.py owns every other reader and writer of this field, so the cluster lives there, with the scanner declared in the mixin's contract block beside the store. Add two e2e tests that heal the record through the monitor's real entry point: apply() flips the ledger and emits the source change, which a direct callback call skips. One covers the plain announce, one the ping-online device that takes apply's all-match branch and moves ownership without a state flip. --- .../controllers/devices/controller.py | 37 +---------------- .../controllers/devices/metadata.py | 40 ++++++++++++++++++- tests/e2e/test_rename_keeps_ota_reachable.py | 40 +++++++++++++++++++ 3 files changed, 80 insertions(+), 37 deletions(-) diff --git a/esphome_device_builder/controllers/devices/controller.py b/esphome_device_builder/controllers/devices/controller.py index 5056fa9913..df46e667ca 100644 --- a/esphome_device_builder/controllers/devices/controller.py +++ b/esphome_device_builder/controllers/devices/controller.py @@ -84,7 +84,7 @@ ) from ._ignored_devices_store import SAVE_DELAY as _IGNORED_DEVICES_SAVE_DELAY from ._ignored_devices_store import ignored_devices_store -from ._metadata_store import _DEFAULT_SAVE_DELAY, DeviceMetadataStore +from ._metadata_store import DeviceMetadataStore from ._pending_keys_store import PendingKeysStore from ._shared_sidecar import SharedSidecarClient from ._state import DevicesState @@ -1328,41 +1328,6 @@ async def _sync_deployed_state_after_flash(self, configuration: str) -> None: def _schedule_version_reprobe(self, configuration: str) -> None: firmware_sync.schedule_version_reprobe(self, configuration) - def _deployed_name(self, device: Device) -> str: - """Return *device*'s recorded hostname, or ``""`` while another config owns it.""" - deployed = device.deployed_name - return "" if not deployed or self._devices_by_name(deployed) else deployed - - def _stamp_deployed_name( - self, configuration: str, *, old_name: str, new_name: str, device: Device | None = None - ) -> str: - """ - Record the hostname the firmware answers to; returns it (``""`` if cleared). - - A chained rename keeps the existing record; renaming back to it clears. - """ - current = str(self._metadata_store.get_field(configuration, "deployed_name") or old_name) - deployed = "" if current == new_name else current - self._set_deployed_name(configuration, deployed, device=device) - return deployed - - def _clear_deployed_name(self, configuration: str, *, device: Device | None = None) -> None: - """Forget the recorded hostname; the firmware carries the YAML's own name.""" - if self._metadata_store.get_field(configuration, "deployed_name"): - self._set_deployed_name(configuration, "", device=device) - - def _set_deployed_name( - self, configuration: str, deployed: str, *, device: Device | None = None - ) -> None: - """Write the record to the live row (*device*, or the indexed one) and the store.""" - if device is None: - device = self._scanner.get_by_configuration(configuration) - if device is not None: - device.deployed_name = deployed - # A lost stamp strands the device; a lost clear self-heals on the next announce. - delay = 0.0 if deployed else _DEFAULT_SAVE_DELAY - self._metadata_store.update(configuration, deployed_name=deployed, delay=delay) - def _cancel_reprobe_timers(self) -> None: """Cancel any pending post-flash re-probe timers.""" for handle in self._reprobe_timers.values(): diff --git a/esphome_device_builder/controllers/devices/metadata.py b/esphome_device_builder/controllers/devices/metadata.py index 72092b9a98..9260565357 100644 --- a/esphome_device_builder/controllers/devices/metadata.py +++ b/esphome_device_builder/controllers/devices/metadata.py @@ -15,9 +15,11 @@ from ...helpers.metadata_sidecar import metadata_transaction from .._device_builder_base import DeviceBuilderBase from .._device_scanner import DeviceFileMetadata, MetadataResolver -from ._metadata_store import STORE_FIELDS +from ._metadata_store import _DEFAULT_SAVE_DELAY, STORE_FIELDS if TYPE_CHECKING: + from ...models import Device + from .._device_scanner import DeviceScanner from ..boards import BoardCatalog from ._metadata_store import DeviceMetadataStore from ._shared_sidecar import SharedSidecarClient @@ -86,6 +88,7 @@ class DeviceMetadataBase(DeviceBuilderBase): # Subclass (``DevicesController``) populates these in ``__init__``. _metadata_store: DeviceMetadataStore _shared_sidecar: SharedSidecarClient + _scanner: DeviceScanner def _resolve_device_metadata( self, config_dir: Path, filename: str, shared_md: dict[str, Any] | None = None @@ -220,6 +223,41 @@ async def _migrate_device_metadata( await self._metadata_store.rename(old_configuration, new_configuration) await self._shared_sidecar.rename(old_configuration, new_configuration) + def _deployed_name(self, device: Device) -> str: + """Return *device*'s recorded hostname, or ``""`` while another config owns it.""" + deployed = device.deployed_name + return "" if not deployed or self._scanner.get_by_name(deployed) else deployed + + def _stamp_deployed_name( + self, configuration: str, *, old_name: str, new_name: str, device: Device | None = None + ) -> str: + """ + Record the hostname the firmware answers to; returns it (``""`` if cleared). + + A chained rename keeps the existing record; renaming back to it clears. + """ + current = str(self._metadata_store.get_field(configuration, "deployed_name") or old_name) + deployed = "" if current == new_name else current + self._set_deployed_name(configuration, deployed, device=device) + return deployed + + def _clear_deployed_name(self, configuration: str, *, device: Device | None = None) -> None: + """Forget the recorded hostname; the firmware carries the YAML's own name.""" + if self._metadata_store.get_field(configuration, "deployed_name"): + self._set_deployed_name(configuration, "", device=device) + + def _set_deployed_name( + self, configuration: str, deployed: str, *, device: Device | None = None + ) -> None: + """Write the record to the live row (*device*, or the indexed one) and the store.""" + if device is None: + device = self._scanner.get_by_configuration(configuration) + if device is not None: + device.deployed_name = deployed + # A lost stamp strands the device; a lost clear self-heals on the next announce. + delay = 0.0 if deployed else _DEFAULT_SAVE_DELAY + self._metadata_store.update(configuration, deployed_name=deployed, delay=delay) + async def _clear_volatile_device_metadata(self, configuration: str) -> None: """Clear archive-volatile fields in both stores (keeps identity). diff --git a/tests/e2e/test_rename_keeps_ota_reachable.py b/tests/e2e/test_rename_keeps_ota_reachable.py index 305e5b1b2a..dc254ceb03 100644 --- a/tests/e2e/test_rename_keeps_ota_reachable.py +++ b/tests/e2e/test_rename_keeps_ota_reachable.py @@ -104,3 +104,43 @@ async def test_config_only_rename_then_install_targets_the_old_hostname( assert device is not None assert device.deployed_name == "" assert "deployed_name" not in db.devices._metadata_store.get("livingroom.yaml") + + +async def test_announce_under_the_new_name_heals_the_record(dashboard: DeviceBuilder) -> None: + """A flash from outside the dashboard: the device announces its new name (#2730).""" + db = dashboard + assert db.devices is not None + monitor = db.devices._state_monitor + await db.devices.rename_device( + configuration="kitchen.yaml", new_name="livingroom", config_only=True + ) + device = db.devices.get_by_configuration("livingroom.yaml") + assert device is not None and device.deployed_name == "kitchen" + + # The production entry point: the monitor's ledger flips to mdns and emits the + # source change; a direct callback call would skip that gate. + monitor.apply("livingroom", DeviceState.ONLINE, "mdns", claim=True) + + assert device.deployed_name == "" + assert "deployed_name" not in db.devices._metadata_store.get("livingroom.yaml") + + +async def test_ping_online_device_still_heals_on_the_mdns_claim( + dashboard: DeviceBuilder, +) -> None: + """Already ONLINE via ping, so ``apply`` takes its all-match branch: ownership only.""" + db = dashboard + assert db.devices is not None + monitor = db.devices._state_monitor + await db.devices.rename_device( + configuration="kitchen.yaml", new_name="livingroom", config_only=True + ) + device = db.devices.get_by_configuration("livingroom.yaml") + assert device is not None + monitor.apply("livingroom", DeviceState.ONLINE, "ping") + assert device.deployed_name == "kitchen" + + monitor.apply("livingroom", DeviceState.ONLINE, "mdns", claim=True) + + assert device.deployed_name == "" + assert "deployed_name" not in db.devices._metadata_store.get("livingroom.yaml") From 7bf8a2c781b7471909499c38ae92733ed7519410 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 22 Sep 2026 11:20:20 +0100 Subject: [PATCH 15/15] Close the record's remaining silent-failure paths The clear gated on the store while both readers consult the row, so a scanner load that read the store before a clear and swapped its row in after left every later clear a no-op; the guard now honours the row it was handed. The rename re-asserts the stamp under the new filename after the metadata migration, which logs and continues on failure and lets a stale target entry win its merge. Clears write immediately too: the write only fires when a record existed, and a device the mDNS self-heal can't reach had no way back from one lost in the debounce. The bootloader carve-out is scoped to UPLOAD like the CLI flag, and a redirect logs the hostname it targeted so a wrong-board flash is diagnosable afterwards. --- .../controllers/devices/firmware_sync.py | 3 ++- .../controllers/devices/helpers.py | 7 +++++++ .../controllers/devices/metadata.py | 14 +++++++++----- .../controllers/devices/mutations_simple.py | 3 ++- .../devices/test_rename_config_only.py | 15 +++++++++++++++ tests/controllers/firmware/test_address_cache.py | 12 +++++++++--- tests/controllers/firmware/test_refresh.py | 12 ++++++++++++ tests/test_device_state_event.py | 10 ++++++++++ 8 files changed, 66 insertions(+), 10 deletions(-) diff --git a/esphome_device_builder/controllers/devices/firmware_sync.py b/esphome_device_builder/controllers/devices/firmware_sync.py index a6556188ab..05237f5c12 100644 --- a/esphome_device_builder/controllers/devices/firmware_sync.py +++ b/esphome_device_builder/controllers/devices/firmware_sync.py @@ -90,7 +90,8 @@ def on_job_completed(controller: DevicesController, event: Event[JobLifecycleDat return recompute_hash = job_type in COMPILING_JOB_TYPES flashed = job_type in (JobType.UPLOAD, JobType.INSTALL) - if flashed and not job.flash_bootloader: + bootloader_only = job_type is JobType.UPLOAD and job.flash_bootloader + if flashed and not bootloader_only: # A bootloader-only upload replaces no app, so its record stands. # Cleared here, not in the background refresh, so a following job's # address-cache read can't race it. diff --git a/esphome_device_builder/controllers/devices/helpers.py b/esphome_device_builder/controllers/devices/helpers.py index 07f73f5835..668d2438ca 100644 --- a/esphome_device_builder/controllers/devices/helpers.py +++ b/esphome_device_builder/controllers/devices/helpers.py @@ -507,6 +507,13 @@ def _build_address_cache_args( if not cached and is_local and deployed_name: # Publish the pre-rename name's IPs under the key the CLI looks up. cached = monitor.mdns.get_cached_addresses(default_mdns_address(deployed_name)) + if cached: + _LOGGER.info( + "OTA for %s targets %s, the hostname its firmware still answers to (%s)", + normalized, + deployed_name, + ", ".join(cached), + ) if cached: addresses = list(cached) diff --git a/esphome_device_builder/controllers/devices/metadata.py b/esphome_device_builder/controllers/devices/metadata.py index 9260565357..042511b278 100644 --- a/esphome_device_builder/controllers/devices/metadata.py +++ b/esphome_device_builder/controllers/devices/metadata.py @@ -15,7 +15,7 @@ from ...helpers.metadata_sidecar import metadata_transaction from .._device_builder_base import DeviceBuilderBase from .._device_scanner import DeviceFileMetadata, MetadataResolver -from ._metadata_store import _DEFAULT_SAVE_DELAY, STORE_FIELDS +from ._metadata_store import STORE_FIELDS if TYPE_CHECKING: from ...models import Device @@ -243,7 +243,12 @@ def _stamp_deployed_name( def _clear_deployed_name(self, configuration: str, *, device: Device | None = None) -> None: """Forget the recorded hostname; the firmware carries the YAML's own name.""" - if self._metadata_store.get_field(configuration, "deployed_name"): + if device is None: + device = self._scanner.get_by_configuration(configuration) + # Either side may hold it: an executor load can swap a pre-clear row in. + if (device is not None and device.deployed_name) or self._metadata_store.get_field( + configuration, "deployed_name" + ): self._set_deployed_name(configuration, "", device=device) def _set_deployed_name( @@ -254,9 +259,8 @@ def _set_deployed_name( device = self._scanner.get_by_configuration(configuration) if device is not None: device.deployed_name = deployed - # A lost stamp strands the device; a lost clear self-heals on the next announce. - delay = 0.0 if deployed else _DEFAULT_SAVE_DELAY - self._metadata_store.update(configuration, deployed_name=deployed, delay=delay) + # Record writes are rare and a lost one can't always self-heal; don't debounce. + self._metadata_store.update(configuration, deployed_name=deployed, delay=0.0) async def _clear_volatile_device_metadata(self, configuration: str) -> None: """Clear archive-volatile fields in both stores (keeps identity). diff --git a/esphome_device_builder/controllers/devices/mutations_simple.py b/esphome_device_builder/controllers/devices/mutations_simple.py index 9d949e5dad..a11d4cba90 100644 --- a/esphome_device_builder/controllers/devices/mutations_simple.py +++ b/esphome_device_builder/controllers/devices/mutations_simple.py @@ -339,11 +339,12 @@ def _land() -> None: for name in sorted({os.path.normpath(n) for n in (configuration, new_filename)}): await locks.enter_async_context(controller._yaml_write_lock(name)) await run_in_executor(_land) - # Before the migrate, so a moved file carries the record with it. deployed_name = controller._stamp_deployed_name( configuration, old_name=old_name, new_name=new_name ) await migrate_metadata(controller, configuration, new_filename) + # The migration is best-effort and its merge lets a stale target entry win. + controller._set_deployed_name(new_filename, deployed_name) await rescan_renamed(controller, new_filename) # An in-place rescan re-stamps the pre-rename name over a record this # rename just cleared. Only the clear is re-asserted, so this can't diff --git a/tests/controllers/devices/test_rename_config_only.py b/tests/controllers/devices/test_rename_config_only.py index 541dcfd413..05deda68ce 100644 --- a/tests/controllers/devices/test_rename_config_only.py +++ b/tests/controllers/devices/test_rename_config_only.py @@ -521,3 +521,18 @@ async def _rescan_fires_scan_change(_controller: object, configuration: str) -> ) assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") + + +async def test_config_only_rename_records_the_name_even_if_the_migration_fails( + tmp_path: Path, make_controller: MakeControllerFactory +) -> None: + """``migrate_metadata`` logs and continues, so the stamp is re-asserted under the new file.""" + controller = make_controller(tmp_path) + (tmp_path / "kitchen.yaml").write_text(_YAML, encoding="utf-8") + controller._migrate_device_metadata = AsyncMock(side_effect=OSError("disk")) + + await controller.rename_device( + configuration="kitchen.yaml", new_name="livingroom", config_only=True + ) + + assert controller._metadata_store.get("livingroom.yaml")["deployed_name"] == "kitchen" diff --git a/tests/controllers/firmware/test_address_cache.py b/tests/controllers/firmware/test_address_cache.py index c9d0c333da..ded1027905 100644 --- a/tests/controllers/firmware/test_address_cache.py +++ b/tests/controllers/firmware/test_address_cache.py @@ -9,9 +9,12 @@ from __future__ import annotations +import logging from typing import Any from unittest.mock import MagicMock +import pytest + from esphome_device_builder.controllers.devices import DevicesController from esphome_device_builder.controllers.devices.helpers import _build_address_cache_args from esphome_device_builder.controllers.firmware import FirmwareController @@ -152,11 +155,14 @@ def test_multiple_cached_addresses_sorted() -> None: # ---------------------------------------------------------------------- -def test_deployed_name_cache_backs_the_yaml_name_key() -> None: - """The pre-rename name's cached IPs are published under the YAML name's key.""" +def test_deployed_name_cache_backs_the_yaml_name_key(caplog: pytest.LogCaptureFixture) -> None: + """The pre-rename name's cached IPs are published under the YAML name's key, and logged.""" monitor = RecordingStateMonitor(cached_addresses={"asistente.local": ["192.168.1.50"]}) - args = _build_address_cache_args(_device(), monitor, "asistente") + with caplog.at_level(logging.INFO): + args = _build_address_cache_args(_device(), monitor, "asistente") assert args == ["--mdns-address-cache", "kitchen.local=192.168.1.50"] + # A redirect that reaches the wrong board must be diagnosable after the fact. + assert "targets asistente" in caplog.text def test_deployed_name_not_consulted_when_own_name_resolves() -> None: diff --git a/tests/controllers/firmware/test_refresh.py b/tests/controllers/firmware/test_refresh.py index 3578c80655..ca5a095b00 100644 --- a/tests/controllers/firmware/test_refresh.py +++ b/tests/controllers/firmware/test_refresh.py @@ -187,6 +187,18 @@ async def test_completed_app_upload_clears_the_deployed_name(tmp_path: Path) -> assert row.deployed_name == "" +async def test_completed_install_clears_even_with_the_bootloader_flag(tmp_path: Path) -> None: + """The CLI honours ``--bootloader`` for UPLOAD only; an INSTALL always flashes the app.""" + controller, _ = _make_controller(tmp_path) + controller._metadata_store.update("kitchen.yaml", deployed_name="asistente", delay=0.0) + job = _job(JobType.INSTALL, JobStatus.COMPLETED) + job.flash_bootloader = True + + controller._on_firmware_job_completed(Event(EventType.JOB_COMPLETED, {"job": job})) + + assert "deployed_name" not in controller._metadata_store.get("kitchen.yaml") + + async def test_completed_bootloader_upload_keeps_the_deployed_name(tmp_path: Path) -> None: """``--bootloader`` replaces no app, so the recorded hostname still stands.""" controller, _ = _make_controller(tmp_path) diff --git a/tests/test_device_state_event.py b/tests/test_device_state_event.py index 3e178ceba4..b1934f6860 100644 --- a/tests/test_device_state_event.py +++ b/tests/test_device_state_event.py @@ -103,6 +103,16 @@ async def test_mdns_ownership_clears_the_deployed_name( assert rows[0].deployed_name == (expected or "") +async def test_mdns_ownership_clears_a_row_the_store_already_lost() -> None: + """An executor load can swap a pre-clear row in after the store was cleared.""" + row = make_device(address="", deployed_name="asistente") + ctrl, _ = make_devices_controller_with_bus([row]) + + ctrl._on_source_change("kitchen", ReachabilitySource.MDNS) + + assert row.deployed_name == "" + + async def test_mdns_ownership_of_a_ping_online_device_still_clears() -> None: """``apply`` skips the state callback when the device is already ONLINE.""" device = make_device(address="", state=DeviceState.ONLINE)