diff --git a/docs/API.md b/docs/API.md index 78a46fd00..d5234b122 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 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 06ac4e001..f47c71085 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -264,7 +264,12 @@ 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, 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 @@ -803,7 +808,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 +821,13 @@ 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`. + +`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_scanner.py b/esphome_device_builder/controllers/_device_scanner.py index 6c3a9d4ac..f023674ce 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 d29ea179e..2e9a4493f 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,11 @@ async def _verify_and_revive(self, device: Device, ip: str, rtt: float) -> None: ) self._record_dial_failure(key) return - if reported != device.name: + # 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 = "" + 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/_metadata_store.py b/esphome_device_builder/controllers/devices/_metadata_store.py index 5b1dbd154..7f4e88891 100644 --- a/esphome_device_builder/controllers/devices/_metadata_store.py +++ b/esphome_device_builder/controllers/devices/_metadata_store.py @@ -19,12 +19,16 @@ _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( { "ip", "deployed_config_hash", "deployed_version", + "deployed_name", "queued_update", "api_encryption_active", "expected_config_hash", @@ -109,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 c3416b950..df46e667c 100644 --- a/esphome_device_builder/controllers/devices/controller.py +++ b/esphome_device_builder/controllers/devices/controller.py @@ -428,7 +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) + 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).""" diff --git a/esphome_device_builder/controllers/devices/firmware_sync.py b/esphome_device_builder/controllers/devices/firmware_sync.py index 9ca18be6e..05237f5c1 100644 --- a/esphome_device_builder/controllers/devices/firmware_sync.py +++ b/esphome_device_builder/controllers/devices/firmware_sync.py @@ -90,6 +90,12 @@ 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) + 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. + 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. @@ -233,6 +239,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.""" + # 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 e07361819..668d2438c 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,10 @@ 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 have for *device* or *deployed_name*.""" address = device.address if not address: return [] @@ -502,6 +504,16 @@ def _build_address_cache_args(device: Device, monitor: DeviceStateMonitor | None 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: + _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 897adb48b..042511b27 100644 --- a/esphome_device_builder/controllers/devices/metadata.py +++ b/esphome_device_builder/controllers/devices/metadata.py @@ -18,6 +18,8 @@ from ._metadata_store import 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 @@ -126,6 +129,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,6 +143,7 @@ def _resolve_device_metadata( deployed_version=deployed_version, queued_update=queued_update, api_encryption_active=api_encryption_active, + deployed_name=deployed_name, ) def _make_metadata_resolver(self) -> MetadataResolver: @@ -218,6 +223,45 @@ 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 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( + 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 + # 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 855a1615c..a11d4cba9 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,8 +339,18 @@ 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) + 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 + # overwrite a fresher mDNS or concurrent-rename value. + if not 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 a63acb136..a092ba240 100644 --- a/esphome_device_builder/controllers/devices/scan_change.py +++ b/esphome_device_builder/controllers/devices/scan_change.py @@ -152,13 +152,20 @@ 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 or previous.name == device.name ): return + # A hand-edited name strands the firmware like a config-only rename. + # 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, device=device + ) # 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 b0d9f5434..e6ebba9cc 100644 --- a/esphome_device_builder/controllers/devices/state_callbacks.py +++ b/esphome_device_builder/controllers/devices/state_callbacks.py @@ -80,8 +80,13 @@ 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.""" - for device in controller._devices_by_name(name): + """Update ``active_source`` and fire DEVICE_UPDATED; also clears ``deployed_name``.""" + devices = controller._devices_by_name(name) + # 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, device=devices[0]) + for device in devices: if device.runtime_state.active_source == source: continue device.runtime_state.active_source = source diff --git a/esphome_device_builder/helpers/device_yaml/_loading.py b/esphome_device_builder/helpers/device_yaml/_loading.py index d307db99b..63b5c7ee4 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 49ff9b04e..d1a64fd46 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 = 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/conftest.py b/tests/conftest.py index fd4c89470..8aaff872b 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 @@ -957,6 +958,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], *, @@ -977,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 @@ -997,24 +1007,18 @@ 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 = 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. 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/devices/test_branches_coverage.py b/tests/controllers/devices/test_branches_coverage.py index 8c92a0540..120e0b932 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 [], ) @@ -1614,7 +1621,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 +1638,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 +1655,45 @@ 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) + + 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( + tmp_path: Path, make_controller: MakeControllerFactory +) -> None: + """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") + + controller._on_scan_change(ScanChange.RELOADED, refined, _device("livingroom-yaml")) + + assert controller._metadata_store.get(refined.configuration) == {} + + +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 +1711,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 +1725,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/devices/test_metadata_store.py b/tests/controllers/devices/test_metadata_store.py index ec2ae2fc5..6fdd0c1b3 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 b785076ab..05deda68c 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 @@ -446,3 +448,91 @@ 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 + + +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") + + +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/devices/test_rename_metadata.py b/tests/controllers/devices/test_rename_metadata.py index 0b423cf77..881f8bf65 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 bd8970cbb..ded102790 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 @@ -42,9 +45,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 +149,77 @@ 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(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"]}) + 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: + """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_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"]}) + 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, + monitor: RecordingStateMonitor | None = None, + names_taken: tuple[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, 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() + 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"]) return controller @@ -248,6 +303,31 @@ 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"), + monitor=RecordingStateMonitor(cached_addresses={"asistente.local": ["10.0.0.1"]}), + names_taken=("asistente",), + ) + + 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 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"]}), + ) + + 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()) diff --git a/tests/controllers/firmware/test_refresh.py b/tests/controllers/firmware/test_refresh.py index 00f9ef0b9..ca5a095b0 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: @@ -115,7 +114,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 +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] + attach_metadata_store(controller, tmp_path) return controller, captured @@ -157,9 +157,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 +170,50 @@ 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; row and store both clear (#2730).""" + 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 "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 == "" + + +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) + 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 +224,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 +237,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 +247,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 +258,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 +267,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 +277,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 +292,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 +302,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, @@ -302,12 +343,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) @@ -336,15 +372,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) @@ -443,15 +473,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 diff --git a/tests/e2e/conftest.py b/tests/e2e/conftest.py index 746ab93bf..8b7cec02e 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 89d737d90..047948931 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 new file mode 100644 index 000000000..dc254ceb0 --- /dev/null +++ b/tests/e2e/test_rename_keeps_ota_reachable.py @@ -0,0 +1,146 @@ +"""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, 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 = """\ +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: + async with single_device_dashboard(make_settings, tmp_path, _YAML) as db: + yield db + + +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 + 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 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 == "" + 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") diff --git a/tests/test_api_reviver.py b/tests/test_api_reviver.py index d7fbb9d45..ebf671c35 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, ) @@ -353,6 +354,33 @@ 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_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 a135c6f4d..b1934f686 100644 --- a/tests/test_device_state_event.py +++ b/tests/test_device_state_event.py @@ -10,7 +10,11 @@ from __future__ import annotations -from esphome_device_builder.models import DeviceState, EventType +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 @@ -58,3 +62,64 @@ def test_state_change_unknown_device_does_not_fire() -> None: ctrl._on_state_change("ghost", DeviceState.ONLINE, "mdns") assert captured == [] + + +@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="", 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_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) + 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")