fix(review): fix(review): pick station device by entity count, drop ghost devices

custom_components/sws12500/predecessor.py

@@ -182,17 +182,42 @@ def _adopt_devices(hass: HomeAssistant, entry: ConfigEntry, old_entry: ConfigEnt
 
     A predecessor entry can own more than one device (v2.0.0pre1 created a second one per
     Ecowitt channel), and only one device may hold a given identifier: passing the same
-    set twice raises `DeviceIdentifierCollisionError` and would fail setup for good. The
-    extra devices therefore keep their own identifiers and are only repointed, which
-    leaves them attached to this entry and reapable once they hold nothing.
+    set twice raises `DeviceIdentifierCollisionError` and would fail setup for good. So
+    exactly one of them can be the device our platforms resolve, and which one must not
+    be left to the order the registry happens to yield. Once the domain really changes,
+    nothing holds the target identifiers yet, so a per-channel row that happened to come
+    first would become the station device and every adopted entity would be moved onto
+    it, stranding the real one with the user's name, area and labels on it. The device
+    carrying the most entities wins instead, ties broken on `device.id` so the outcome
+    does not depend on storage order at all. The others keep their own identifiers and
+    are only repointed.
+
+    A device carrying no entities is left alone entirely. Attaching this entry to it
+    would keep it alive forever: `device_registry.async_cleanup` subtracts
+    `references_config_entries` from the orphan set, so a device linked to any live
+    config entry is never reaped, and an empty device page would sit under this
+    integration for good. Left untouched, `async_remove(old_entry)` takes its last config
+    entry away and deletes it. Nothing rides on it - the remove branch of
+    `entity_registry.async_device_modified` only deletes entities whose `config_entry_id`
+    is one of the removed device's own, and by this point the adopted ones point at this
+    entry.
     """
 
     device_registry = dr.async_get(hass)
+    registry = er.async_get(hass)
     identifiers = set(build_device_info(entry)["identifiers"])
 
-    for device in dr.async_entries_for_config_entry(device_registry, old_entry.entry_id):
+    devices = dr.async_entries_for_config_entry(device_registry, old_entry.entry_id)
+    held = {
+        device.id: len(er.async_entries_for_device(registry, device.id, include_disabled_entities=True))
+        for device in devices
+    }
+    populated = [device for device in devices if held[device.id]]
+    canonical = min(populated, key=lambda device: (-held[device.id], device.id), default=None)
+
+    for device in populated:
         holder = device_registry.async_get_device(identifiers=identifiers)
-        if holder is not None and holder.id != device.id:
+        if device is not canonical or (holder is not None and holder.id != device.id):
             device_registry.async_update_device(device.id, add_config_entry_id=entry.entry_id)
             continue
 
@@ -280,6 +305,13 @@ async def async_adopt_predecessor(
                     "Restart Home Assistant and try again",
                     old_entry.entry_id,
                 )
+                # Counted as left behind rather than passed over in silence. This is the
+                # run where *nothing* moved, so it is also where deleting the old entry
+                # by hand would cost the most - exactly what the repair notice is for.
+                registry = er.async_get(hass)
+                result.live.extend(
+                    old.entity_id for old in er.async_entries_for_config_entry(registry, old_entry.entry_id)
+                )
                 continue
 
         # Scoped to this predecessor entry, then folded in: whether an entry may be

tests/test_predecessor_adoption.py

@@ -365,6 +365,80 @@ async def test_a_second_device_does_not_collide(hass: HomeAssistant, entries) ->
     assert new.entry_id in kept.config_entries
 
 
+async def test_the_station_device_wins_regardless_of_registry_order(hass: HomeAssistant, entries) -> None:
+    """Which device becomes ours must not be decided by storage order.
+
+    Registered here in the order that punishes "first one claims the identifiers": the
+    single-entity channel row comes first. After the rename nothing holds the target
+    identifiers yet, so nothing else would stop it - and the station device, carrying the
+    user's own name, area and labels, would be stranded on the old-domain identifier
+    while every adopted entity is moved onto the channel row.
+    """
+    old, new = entries
+    device_registry = dr.async_get(hass)
+    channel = device_registry.async_get_or_create(
+        config_entry_id=old.entry_id,
+        identifiers={(OLD_DOMAIN, "ecowitt_ch1")},
+        name="Channel 1",
+    )
+    station = device_registry.async_get_or_create(
+        config_entry_id=old.entry_id,
+        identifiers={(OLD_DOMAIN, "station")},
+        name="Weather Station",
+    )
+    ordered = [d.id for d in dr.async_entries_for_config_entry(device_registry, old.entry_id)]
+    assert ordered[0] == channel.id, "fixture no longer puts the smaller device first"
+
+    _seed(hass, old, device_id=station.id)
+    registry = er.async_get(hass)
+    registry.async_get_or_create("sensor", OLD_DOMAIN, "ch1_temp", config_entry=old, device_id=channel.id)
+
+    await async_adopt_predecessor(hass, new, predecessor_domain=OLD_DOMAIN)
+
+    holder = device_registry.async_get_device(identifiers=set(build_device_info(new)["identifiers"]))
+    assert holder is not None
+    assert holder.id == station.id, "the device holding most of the entities has to be the one we adopt"
+    stayed = device_registry.async_get(channel.id)
+    assert stayed is not None
+    assert stayed.identifiers == {(OLD_DOMAIN, "ecowitt_ch1")}
+
+
+async def test_an_empty_device_is_not_carried_over(hass: HomeAssistant, entries) -> None:
+    """An emptied device must not be kept alive by us, or it never goes away.
+
+    `device_registry.async_cleanup` subtracts every device referenced by a live config
+    entry from the orphan set, so attaching this entry to a device that holds nothing
+    pins it in place permanently - an empty device page under the new integration that
+    no cleanup pass will ever reap. Leaving it alone lets `async_remove(old_entry)` take
+    its last config entry and delete it.
+    """
+    old, new = entries
+    device_registry = dr.async_get(hass)
+    station = device_registry.async_get_or_create(
+        config_entry_id=old.entry_id,
+        identifiers={(OLD_DOMAIN,)},  # type: ignore[arg-type]
+        name="Weather Station",
+    )
+    ghost = device_registry.async_get_or_create(
+        config_entry_id=old.entry_id,
+        identifiers={(OLD_DOMAIN, "ecowitt_ch1")},
+        name="Channel 1",
+    )
+    created = _seed(hass, old, device_id=station.id)
+
+    result = await async_adopt_predecessor(hass, new, predecessor_domain=OLD_DOMAIN)
+
+    assert result.complete
+    assert device_registry.async_get(ghost.id) is None, "the empty device outlived the migration"
+    survived = device_registry.async_get(station.id)
+    assert survived is not None
+    registry = er.async_get(hass)
+    for entity_id in created.values():
+        entry = registry.async_get(entity_id)
+        assert entry is not None
+        assert entry.device_id == station.id
+
+
 # ---------------------------------------------------------------------------
 # More than one predecessor entry
 # ---------------------------------------------------------------------------
@@ -419,6 +493,38 @@ async def test_a_stalled_migration_raises_a_repair_issue(hass: HomeAssistant, en
     assert issue.translation_placeholders == {"entities": blocked}
 
 
+async def test_a_predecessor_that_will_not_unload_raises_a_repair_issue(
+    hass: HomeAssistant, entries, monkeypatch
+) -> None:
+    """The run where nothing at all moved is the one that most needs the warning.
+
+    Passed over in silence, an unloadable predecessor contributes nothing to the result,
+    so `attempted` stays False and the notice is deleted instead of raised - leaving the
+    user with a leftover entry, no explanation, and the one irreversible action a click
+    away.
+    """
+    from homeassistant.config_entries import ConfigEntryState
+
+    old, new = entries
+    created = _seed(hass, old)
+    old.mock_state(hass, ConfigEntryState.LOADED)
+
+    async def _refuse(*_a: Any, **_kw: Any) -> bool:
+        return False
+
+    monkeypatch.setattr(hass.config_entries, "async_unload", _refuse)
+
+    result = await async_adopt_predecessor(hass, new, predecessor_domain=OLD_DOMAIN)
+    update_predecessor_adoption_issue(hass, new, result)
+
+    assert not result.adopted
+    assert sorted(result.live) == sorted(created.values())
+    assert not result.complete
+    issue = ir.async_get(hass).async_get_issue(DOMAIN, f"predecessor_adoption_{new.entry_id}")
+    assert issue is not None
+    assert issue.translation_placeholders == {"entities": ", ".join(sorted(created.values()))}
+
+
 async def test_a_finished_migration_raises_no_issue(hass: HomeAssistant, entries) -> None:
     """Nothing was left behind, so there is nothing to warn about."""
     old, new = entries