diff --git a/custom_components/localthings/config_flow.py b/custom_components/localthings/config_flow.py index 304e83a..e561503 100644 --- a/custom_components/localthings/config_flow.py +++ b/custom_components/localthings/config_flow.py @@ -895,6 +895,28 @@ class LocalThingsConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): _LOGGER.exception("Unexpected error during device probe") errors["base"] = "unknown" else: + # An entry created before v4 still carries the serial-keyed + # unique_id until its first *live* poll adopts the UUID + # (coordinator._resolve_identity) -- which can be a long + # while for an appliance that is off, since an entry loads + # from its snapshot in the meantime (issue #295). The UUID + # check below can't see such an entry, so re-adding this + # very appliance during that window would be waved through + # as a second entry; the two would then collide the moment + # the older one re-keyed, and rekey_entry resolves a + # collision by *deleting* the duplicate rows -- taking the + # original entry's entity_ids, history and automations with + # them. Matched on the legacy key together with the host, so + # issue #381's two units (same serial, different addresses) + # stay separable. + legacy_unique_id = f"localthings_{info['serial']}" + if any( + other.unique_id == legacy_unique_id + and other.data.get(CONF_HOST) == self._host + and CONF_DEVICE_KEY not in other.data + for other in existing + ): + return self.async_abort(reason="already_configured") # Keyed on the OCF device UUID rather than the serialNum # (issue #381): two units of a model that ship the same # well-formed serial are indistinguishable here otherwise, diff --git a/custom_components/localthings/coordinator.py b/custom_components/localthings/coordinator.py index 6c85954..4a7dd09 100644 --- a/custom_components/localthings/coordinator.py +++ b/custom_components/localthings/coordinator.py @@ -1124,23 +1124,27 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): * A snapshot replay (issue #295) never re-keys. That path did not reach the device, so its "reported" identity is only whatever the snapshot happened to preserve. - * Nothing stored (a pre-v4 entry, or a legacy migration that could - not recover an identity) -- adopt what the device reports and - rewrite the registries off the legacy key onto it. This is the - one-time move onto the OCF device UUID (issue #381), deferred to - here because the UUID is only readable from the device itself. + * The polled identity matches what this entry is already keyed on -- + the overwhelmingly common case, and a no-op beyond recording it. * A key is stored and this poll produced no UUID -- keep it. The device saying nothing is not the device saying something different, and demoting a UUID-keyed entry back onto its serial because one reconnect couldn't read /oic/d would re-key every entity the user has for the duration of an outage. - * A key is stored and the UUID differs. Either this appliance's - `di` rotated (an OCF hard reset is allowed to regenerate it) or a + * The identity differs. Either this is the registered appliance + under a new identity -- the one-time move onto the OCF device UUID + (issue #381), or a `di` regenerated by an OCF hard reset -- or a *different* appliance now answers on this address. The serialNum - tells them apart: a usable one that still matches what this entry - was registered with means same unit, new UUID, so follow it. - Anything else keeps the registered identity and warns -- re-adding - is the user's call, not something a poll gets to decide. + is what tells those apart. + + That last decision is deliberately the same whether or not this + entry has adopted a UUID yet. A pre-v4 entry is the population this + change exists to move, but it is also the population that has been + running longest, so it is the last one that should lose its + registry rows to an appliance that merely happens to share its + address -- a guard `_run_discovery` has had since issue #236 and + which an unconditional "adopt whatever the device says" would have + quietly dropped for exactly those users. """ host = self._entry.data[CONF_HOST] stored_serial = self._entry.data.get(CONF_SERIAL) @@ -1149,31 +1153,25 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): polled_ocf = ocf_device_key(self._identity) stored_key = self._entry.data.get(CONF_DEVICE_KEY) + # What this entry's registry rows actually carry today. A pre-v4 + # entry has no CONF_DEVICE_KEY, so it is whatever the coordinator + # has always fallen back to -- see this class's __init__. + current_key = stored_key if stored_key is not None else (stored_serial or host) + # `polled_serial` is already resolve_serial's output, so this is + # exactly resolve_device_key's chain: the UUID where the device + # reports one, today's serial/host answer where it doesn't. + polled_key = polled_ocf or polled_serial - if stored_key is None: - # `polled_serial` is already resolve_serial's output, so this is - # exactly resolve_device_key's chain: the UUID where the device - # reports one, today's serial/host answer where it doesn't. - polled_key = polled_ocf or polled_serial - legacy_key = stored_serial or host - if legacy_key != polled_key: - self._log.info( - "moving this device's registry entries from %r onto %r", - legacy_key, - polled_key, - ) - rekey_entry(self.hass, self._entry, legacy_key, polled_key) - return polled_key, polled_serial + if polled_key == current_key: + # Returned rather than short-circuited so a pre-v4 entry records + # the key it has always had, which is what stops the next poll + # from treating this same answer as a change. + return current_key, polled_serial - if polled_ocf == stored_key: - # Confirmed the registered appliance, so this poll's serial is - # authoritative -- firmware is allowed to correct it. - return stored_key, polled_serial - if polled_ocf is None: + if stored_key is not None and polled_ocf is None: # Nothing to compare against: this may or may not be the # registered appliance, so neither half is updated. return stored_key, stored_serial or polled_serial - polled_key = polled_ocf # Excluding the host answer is what keeps this from firing on two # *different* placeholder-serial units: `polled_serial` is already @@ -1182,25 +1180,27 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): same_unit = polled_serial == stored_serial and polled_serial != host # An entry keyed on the host never made an identity claim to defend # -- it is registered against "whatever answers at this address" - # (issues #83/#189). Any real UUID is a strict improvement on that, - # so it is adopted without needing the serial to corroborate it; - # requiring corroboration would strand exactly the placeholder-serial - # boards this whole change is meant to rescue, since their serial - # resolves to the host and can never match. - if not (stored_key == host or same_unit): + # (issues #83/#189). Any real identity is a strict improvement on + # that, so it is adopted without needing the serial to corroborate + # it; requiring corroboration would strand exactly the + # placeholder-serial boards this whole change is meant to rescue, + # since their serial resolves to the host and can never match. An + # entry with no stored serial at all has nothing to corroborate + # against either, and is treated the same way. + if not (current_key == host or same_unit or stored_serial is None): self._log.warning( "device at %s identifies as %r but this entry is registered as %r " "(serial %r, registered %r); keeping the registered identity", host, polled_key, - stored_key, + current_key, polled_serial, stored_serial, ) - return stored_key, stored_serial or polled_serial + return current_key, stored_serial or polled_serial - # Corroborated: same unit with a new UUID (a factory reset is - # allowed to regenerate `di`), or an address-keyed entry finally + # Corroborated: a pre-v4 entry moving onto its UUID, the same unit + # with a regenerated `di`, or an address-keyed entry finally # learning a real identity. Following it keeps the user's # entity_ids, history and automations rather than stranding them on # a key the device will never report again. @@ -1208,10 +1208,10 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): "device %s (serial %r) changed key from %r to %r; following it", host, polled_serial, - stored_key, + current_key, polled_key, ) - rekey_entry(self.hass, self._entry, stored_key, polled_key) + rekey_entry(self.hass, self._entry, current_key, polled_key) return polled_key, polled_serial def _persist_identity( @@ -1232,7 +1232,8 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): `device_key` is what the registries are keyed on; `serial` is stored alongside it rather than replaced by it, because it is what - `_resolve_key` corroborates a changed UUID against on a later poll. + `_resolve_identity` corroborates a changed UUID against on a later + poll. None leaves whatever key is already stored untouched -- see the caller for why a snapshot replay must not write one. @@ -1325,8 +1326,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): name=ident.get("name") or "", serial=ident.get("serial"), # Absent from a snapshot written before these fields - # existed; _resolve_key never re-keys from a replay, so - # a missing UUID here costs nothing beyond diagnostics. + # existed; _resolve_identity never re-keys from a + # replay, so a missing UUID here costs nothing beyond + # diagnostics. device_id=ident.get("device_id"), platform_id=ident.get("platform_id"), device_types=tuple(ident.get("device_types") or ()), @@ -1500,9 +1502,9 @@ class LocalThingsCoordinator(DataUpdateCoordinator[dict[str, Any]]): # A snapshot replay passes None: it never reached the device, so it # has no standing to write a device key. Persisting `self.device_key` # there would freeze a pre-v4 entry's legacy key into CONF_DEVICE_KEY - # as if a poll had confirmed it, and _resolve_key would then treat - # the real UUID -- when a live poll finally produced one -- as a - # changed identity to defend against rather than the one-time + # as if a poll had confirmed it, and _resolve_identity would then + # treat the real UUID -- when a live poll finally produced one -- as + # a changed identity to defend against rather than the one-time # adoption it is. self._persist_identity(None if from_snapshot else key, serial, model, mfr, device_type_name) if not from_snapshot: diff --git a/custom_components/localthings/rekey.py b/custom_components/localthings/rekey.py index db04631..415b878 100644 --- a/custom_components/localthings/rekey.py +++ b/custom_components/localthings/rekey.py @@ -64,9 +64,9 @@ def rekey_entry(hass: HomeAssistant, entry: ConfigEntry, old_key: str, new_key: if old_key == new_key: return - new_unique_id = f"{DOMAIN}_{new_key}" - if entry.unique_id != new_unique_id: - hass.config_entries.async_update_entry(entry, unique_id=new_unique_id) + new_entry_unique_id = f"{DOMAIN}_{new_key}" + if entry.unique_id != new_entry_unique_id: + hass.config_entries.async_update_entry(entry, unique_id=new_entry_unique_id) ent_reg = er.async_get(hass) stale_prefix = f"{DOMAIN}_{old_key}_" diff --git a/tests/localthings/test_config_flow.py b/tests/localthings/test_config_flow.py index afd6596..09ec45a 100644 --- a/tests/localthings/test_config_flow.py +++ b/tests/localthings/test_config_flow.py @@ -1115,6 +1115,72 @@ async def test_same_serial_on_two_units_is_not_a_duplicate(hass: HomeAssistant, assert result["data"][CONF_SERIAL] == first.data[CONF_SERIAL] +async def test_re_adding_during_the_migration_window_is_still_a_duplicate( + hass: HomeAssistant, mock_probe +) -> None: + """An entry created before v4 keeps its serial-keyed unique_id until its + first *live* poll adopts the UUID, which can be a long while for an + appliance that is off (it loads from its snapshot meanwhile, issue + #295). The UUID check can't see such an entry, so without a second + check on the legacy key, re-adding this very appliance in that window + would be waved through -- and the two entries would collide the moment + the older one re-keyed, with rekey_entry resolving the collision by + deleting the duplicate rows and taking the original's entity_ids, + history and automations with them. + """ + existing = MockConfigEntry( + domain=DOMAIN, + data={k: v for k, v in ENTRY_DATA.items() if k != CONF_DEVICE_KEY}, + unique_id=f"localthings_{MOCK_SERIAL}", + version=3, + ) + existing.add_to_hass(hass) + + result = await hass.config_entries.flow.async_init(DOMAIN, context={"source": "user"}) + result = await hass.config_entries.flow.async_configure( + result["flow_id"], {CONF_HOST: ENTRY_DATA[CONF_HOST]} + ) + + assert result["type"] == FlowResultType.ABORT + assert result["reason"] == "already_configured" + + +async def test_the_migration_window_check_still_separates_two_same_serial_units( + hass: HomeAssistant, mock_probe +) -> None: + """The legacy-key check above matches on the host as well as the serial, + so it cannot undo the fix: issue #381's two units share a serial but sit + at different addresses, and the second must still be addable while the + first is mid-migration.""" + existing = MockConfigEntry( + domain=DOMAIN, + data={ + **{k: v for k, v in ENTRY_DATA.items() if k != CONF_DEVICE_KEY}, + CONF_HOST: "192.168.0.3", + }, + unique_id=f"localthings_{MOCK_SERIAL}", + version=3, + ) + existing.add_to_hass(hass) + + second_probe = { + **_probe_result(recognized=True), + "device_key": "3771f8bf-c184-3a2d-d885-e4c9818736d2", + "serial": MOCK_SERIAL, + } + with patch( + "custom_components.localthings.config_flow._probe_and_validate", + return_value=second_probe, + ): + result = await hass.config_entries.flow.async_init(DOMAIN, context={"source": "user"}) + result = await hass.config_entries.flow.async_configure( + result["flow_id"], {CONF_HOST: "192.168.0.14"} + ) + + assert result["type"] == FlowResultType.CREATE_ENTRY + assert result["data"][CONF_DEVICE_KEY] == "3771f8bf-c184-3a2d-d885-e4c9818736d2" + + def test_probe_reads_the_device_key_from_oic_d_without_an_extra_round_trip(monkeypatch): """`_read_device` already fetches /oic/p and /oic/d for the device-type signal, so keying on the OCF UUID costs no additional GET -- it reads diff --git a/tests/localthings/test_identity_migration.py b/tests/localthings/test_identity_migration.py index 6972e47..0ff3971 100644 --- a/tests/localthings/test_identity_migration.py +++ b/tests/localthings/test_identity_migration.py @@ -114,6 +114,14 @@ def _entry( return entry +def _reporting_serial(resources: dict, serial: str) -> dict: + """`resources` with the serialNum the device reports swapped out, so a + test can pair a fixture with the identity its scenario implies.""" + info = dict(resources["/information/vs/0"]) + info["x.com.samsung.da.serialNum"] = serial + return {**resources, "/information/vs/0": info} + + def _seed_registry(hass: HomeAssistant, entry: MockConfigEntry, key: str, **entity_kwargs): """A device and one entity keyed on `key`, as a running install has.""" dev_reg = dr.async_get(hass) @@ -229,10 +237,14 @@ async def test_the_two_units_from_the_issue_migrate_to_separate_identities( including their entity unique_ids, which is where issue #83's Bug 4 silently dropped the second unit's entities even once its entry existed. """ + # Both units report the shared serial, as the real ones do -- so the + # entry's stored identity still matches what the device says, and only + # the UUID separates them. + resources = _reporting_serial(fridge_resources, SHARED_SERIAL) first = _entry(hass, version=3, key=SHARED_SERIAL, host="192.168.0.3") _, first_entity = _seed_registry(hass, first, SHARED_SERIAL, suggested_object_id="purifier_a") - with _reachable(fridge_resources, UUID_A): + with _reachable(resources, UUID_A): await hass.config_entries.async_setup(first.entry_id) await hass.async_block_till_done() @@ -243,7 +255,7 @@ async def test_the_two_units_from_the_issue_migrate_to_separate_identities( second = _entry(hass, version=3, key=SHARED_SERIAL, host="192.168.0.14") _, second_entity = _seed_registry(hass, second, SHARED_SERIAL, suggested_object_id="purifier_b") - with _reachable(fridge_resources, UUID_B): + with _reachable(resources, UUID_B): await hass.config_entries.async_setup(second.entry_id) await hass.async_block_till_done() @@ -453,14 +465,16 @@ async def test_an_offline_load_never_rewrites_the_registry( snapshot says, because that is last run's answer rather than the device's. - Constructed so the two disagree: the snapshot was banked against a - device reporting one serial, while the entry is registered under - another. A replay that trusted the snapshot would rewrite every - registry row to match it -- without the appliance having been reachable - at any point -- and would then freeze that answer into CONF_DEVICE_KEY, - so the real UUID could never be adopted afterwards. + Modelled on a placeholder-serial board (issues #83/#189), which is + where the two can genuinely disagree: the entry is keyed on its address + because the board reports no usable serial, while the snapshot banked + whatever serial the polled resources carried. A replay that trusted the + snapshot would rewrite every registry row onto that serial -- without + the appliance having been reachable at any point -- and would then + freeze the answer into CONF_DEVICE_KEY, so the real UUID could never be + adopted afterwards. """ - entry = _entry(hass, version=3, key=MOCK_SERIAL) + entry = _entry(hass, version=3, key=MOCK_HOST) with _reachable(fridge_resources, None): await hass.config_entries.async_setup(entry.entry_id) @@ -468,30 +482,29 @@ async def test_an_offline_load_never_rewrites_the_registry( await hass.config_entries.async_unload(entry.entry_id) await hass.async_block_till_done() - # The entry is registered under a different identity than the snapshot's, - # and back on the pre-v4 shape the upgrade finds. + # Back to the pre-v4 shape the upgrade finds, still keyed on the address. hass.config_entries.async_update_entry( entry, data={ **{k: v for k, v in entry.data.items() if k != CONF_DEVICE_KEY}, - CONF_SERIAL: "LEGACY-KEY", + CONF_SERIAL: MOCK_HOST, }, - unique_id=f"{DOMAIN}_LEGACY-KEY", + unique_id=f"{DOMAIN}_{MOCK_HOST}", version=3, ) - device, existing = _seed_registry(hass, entry, "LEGACY-KEY") + device, existing = _seed_registry(hass, entry, MOCK_HOST) with _unreachable(): await hass.config_entries.async_setup(entry.entry_id) await hass.async_block_till_done() coordinator = hass.data[DOMAIN][entry.entry_id] - assert coordinator.device_key == "LEGACY-KEY" + assert coordinator.device_key == MOCK_HOST assert CONF_DEVICE_KEY not in entry.data - assert entry.unique_id == f"{DOMAIN}_LEGACY-KEY" - assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, "LEGACY-KEY")} + assert entry.unique_id == f"{DOMAIN}_{MOCK_HOST}" + assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, MOCK_HOST)} assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( - f"{DOMAIN}_LEGACY-KEY_connection_mode" + f"{DOMAIN}_{MOCK_HOST}_connection_mode" ) # And the deferred adoption still works once the appliance answers -- @@ -662,6 +675,39 @@ async def test_a_different_appliance_on_the_same_address_keeps_the_registered_id assert entry.data[CONF_SERIAL] == "SOME-OTHER-APPLIANCE" +async def test_a_pre_v4_entry_defends_itself_against_a_different_appliance( + hass: HomeAssistant, fridge_resources +) -> None: + """The "same IP, different appliance" guard applies to an entry that has + not migrated yet, exactly as it does to one that has. + + A pre-v4 entry is the population this change exists to move, but it is + also the population that has been running longest -- so it is the last + one that should hand its entity_ids, history and automations to an + appliance that merely happens to have taken over its address. Adoption + is the migration's job only when the identity is corroborated. + """ + entry = _entry(hass, version=3, key=MOCK_SERIAL) + device, existing = _seed_registry(hass, entry, MOCK_SERIAL) + + # Neither the serial nor (therefore) the UUID belongs to the registered + # appliance. + resources = _reporting_serial(fridge_resources, "SOME-OTHER-APPLIANCE") + with _reachable(resources, UUID_B): + await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + coordinator = hass.data[DOMAIN][entry.entry_id] + assert coordinator.device_key == MOCK_SERIAL + assert entry.data[CONF_DEVICE_KEY] == MOCK_SERIAL + assert entry.data[CONF_SERIAL] == MOCK_SERIAL + assert entry.unique_id == f"{DOMAIN}_{MOCK_SERIAL}" + assert dr.async_get(hass).async_get(device.id).identifiers == {(DOMAIN, MOCK_SERIAL)} + assert er.async_get(hass).async_get(existing.entity_id).unique_id == ( + f"{DOMAIN}_{MOCK_SERIAL}_connection_mode" + ) + + # --------------------------------------------------------------------------- # rekey_entry's own contract # ---------------------------------------------------------------------------