Address code review: close the migration-window duplicate and unify adoption

Two real findings from review of the identity change.

A pre-v4 entry 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
since the entry loads from its snapshot meanwhile. The config flow's UUID
check couldn't see such an entry, so re-adding the same appliance in that
window was waved through as a second entry -- and the two would collide the
moment the older one re-keyed, with rekey_entry resolving the collision by
deleting the duplicate rows, taking the original's entity_ids, history and
automations with them. The flow now also aborts on the legacy key, matched
together with the host so issue #381's two same-serial units at different
addresses stay separable, and gated on CONF_DEVICE_KEY being absent so the
check disappears once the entry has migrated.

Separately, the "nothing stored yet" branch adopted the polled identity
unconditionally, silently dropping the "same IP, different appliance" guard
_run_discovery has had since issue #236 -- and dropping it for precisely
the users who have been running longest. _resolve_identity now computes the
key an entry currently carries once and applies one corroboration rule to
it, so a pre-v4 entry defends itself exactly as a migrated one does.
Adoption still needs no corroboration where there is no identity claim to
defend: an entry keyed on its address (issues #83/#189) or with no stored
serial at all.

Two tests had to change with it, both because their setup was unfaithful
rather than because the behaviour regressed: the two-unit test now has its
devices report the shared serial they actually report, and the offline test
now models a placeholder-serial board, which is where an entry's key and a
snapshot's serial can genuinely disagree.

Three new tests cover the changed behaviour, and the mutation sweep is
extended to thirteen breakages -- including the two guards added here and
the host match that keeps the duplicate check from undoing #381's fix.

Also from review: rename three stale _resolve_key doc references to
_resolve_identity, and stop rebinding new_unique_id in rekey_entry.
This commit is contained in:
Marc Billow
2026-08-17 05:24:22 +00:00
parent 1e9fbd4ec5
commit c7aa66ef6e
5 changed files with 206 additions and 70 deletions
@@ -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,
+51 -49
View File
@@ -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:
+3 -3
View File
@@ -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}_"
+66
View File
@@ -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
+64 -18
View File
@@ -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
# ---------------------------------------------------------------------------