From ff8dd1c45412904765467894c349178c1c43de55 Mon Sep 17 00:00:00 2001 From: James Mackay Date: Mon, 12 Oct 2026 07:42:38 +0100 Subject: [PATCH] fix(discovery): a cached negative is an answer, not a miss (t57) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep asked `app_label()` whether a package still needed a label, and that accessor answers "what should be displayed" — so a definitive negative (a package whose pm dump ran and carried no label) came back as None, indistinguishable from "never asked". Every sweep therefore re-dumped every label-less package, throwing away the 24 h cache the label path exists for, and a second sweep cost a full dump round for the whole app list. `_known_label(package) -> (known, label)` is the accessor that keeps the two apart: known-and-empty means the device already answered, so the row falls back to its package id with ZERO further round trips. A forced re-sweep now issues only the two list commands. Pinned by tests: a second sweep dumps nothing at all (negatives included), and a package whose fetch FAILED (no output — the UNIT 0 rule) stays uncached and is retried by the next sweep, recovering its label. --- custom_components/nvidia_shield/connection.py | 35 +++++++-- tests/test_installed_apps.py | 72 +++++++++++++++++++ 2 files changed, 102 insertions(+), 5 deletions(-) diff --git a/custom_components/nvidia_shield/connection.py b/custom_components/nvidia_shield/connection.py index f5eba7d..d1a38a4 100644 --- a/custom_components/nvidia_shield/connection.py +++ b/custom_components/nvidia_shield/connection.py @@ -612,6 +612,25 @@ class ShieldConnectionManager: self._app_label_requests.add(package) return None + def _known_label(self, package: str) -> tuple[bool, str]: + """``(known, label)`` from the label cache — "known" = a fetch RAN (t57). + + The accessor the DISCOVERY sweep needs and ``app_label`` deliberately is + not: ``app_label`` answers "what should be displayed", so a definitive + negative (a package with no manifest label) comes back as ``None``, + exactly like a miss. A sweep that used it would re-dump every label-less + package on every run and throw away the 24 h cache it is supposed to + benefit from. Here the two are distinguishable. + """ + cached = self._app_labels.get(package) + if cached is None: + return False, "" + label, stamp = cached + if self._clock() - stamp > APP_LABEL_TTL: + self._app_labels.pop(package, None) # stale — refetch on demand + return False, "" + return True, label + def cache_app_label(self, package: str, label: str | None) -> None: """Record a label fetch that RAN, and what it found (t55, t57). @@ -956,7 +975,7 @@ class ShieldConnectionManager: rows = [ { "package": package, - "name": labels.get(package) or self.app_label(package) or package, + "name": labels.get(package) or package, "icon": icon_for_package(package, APP_ICON_FALLBACKS, APP_ICON_GENERIC), } for package in packages @@ -979,15 +998,21 @@ class ShieldConnectionManager: Cached labels are reused, so a second sweep costs almost nothing; a failed fetch is NOT cached (t57 UNIT 0) and the app falls back to its package id for this table while the next sweep retries it. + + The check goes through :meth:`_known_label` rather than + :meth:`app_label`, so a package the device has definitively answered + "no label" for is not re-dumped on every sweep. """ cached: dict[str, str] = {} missing: list[str] = [] for package in packages: - label = self.app_label(package) - if label: - cached[package] = label - else: + known, label = self._known_label(package) + if not known: missing.append(package) + elif label: + cached[package] = label + # a known NEGATIVE needs no entry: the row falls back to its + # package id, and the cache already holds the definitive answer if not missing: return cached diff --git a/tests/test_installed_apps.py b/tests/test_installed_apps.py index 2be5234..e3b6bc2 100644 --- a/tests/test_installed_apps.py +++ b/tests/test_installed_apps.py @@ -352,6 +352,23 @@ class TestDiscoverySweep: assert manager.installed_apps() == [] assert manager.installed_apps_stale is True + async def test_the_watchdog_never_sweeps(self): + """Acceptance: nothing discovery-related runs on a state poll. + + The watchdog cycle is what a poll drives. It may drain ONE label fetch + (the t24/t55 rule for the live entity path), but it must never list + packages or fan out into a discovery burst. + """ + poller = _DiscoveryPoller() + manager = _manager(poller) + + await manager._watchdog_cycle(5.0) + await asyncio.sleep(0) + + assert not [c for c in poller.shell_calls if "pm list packages" in c] + assert not [c for c in poller.shell_calls if "resolve-activity" in c] + assert manager.installed_apps() == [] + async def test_concurrent_callers_share_one_sweep(self): poller = _DiscoveryPoller() manager = _manager(poller) @@ -421,6 +438,61 @@ class TestDiscoveryDegradesGracefully: assert poller.shell_calls == [] +class TestDiscoveryUsesTheCachedLabelPath: + """Acceptance: labels come from the pm-dump path AND its 24 h cache.""" + + async def test_a_second_sweep_reuses_the_cached_labels(self): + """The 24 h cache is what makes a re-sweep cheap — negatives included. + + A label-less package is a DEFINITIVE answer, so the second sweep must + not re-dump it. The check goes through the cache directly, because the + display accessor (`app_label`) cannot tell "no label" from "not asked". + """ + poller = _DiscoveryPoller(dumps={"ar.tvplayer.tv": DUMP_TIVIMATE}) + manager = _manager(poller) + + await manager.async_refresh_installed_apps() + dumps = [c for c in poller.shell_calls if "pm dump" in c] + assert dumps # the first sweep labelled everything it could + assert len(dumps) >= 5 # ...and it did dump for the unlabelled ones + + # the cache now answers for EVERY package, so a forced re-sweep lists + # again but dumps nothing at all (the 24 h label cache, not the table TTL) + manager._installed_apps = None # forget the table, keep the labels + rows = await manager.async_refresh_installed_apps(force=True) + + assert [c for c in poller.shell_calls if "pm dump" in c] == dumps + assert manager.app_label("ar.tvplayer.tv") == "TiviMate" + by_pkg = {row["package"]: row for row in rows} + assert by_pkg["ar.tvplayer.tv"]["name"] == "TiviMate" + assert by_pkg["org.xbmc.kodi"]["name"] == "org.xbmc.kodi" + + async def test_a_failed_label_is_retried_by_the_next_sweep(self): + """UNIT 0 semantics at scale: a transient failure is not permanent.""" + poller = _DiscoveryPoller(dumps={"org.xbmc.kodi": "application-label: 'Kodi'"}) + manager = _manager(poller) + real_shell = poller.shell + state = {"fail_kodi": True} + + async def _flaky(cmd): + if state["fail_kodi"] and "pm dump" in cmd and "kodi" in cmd: + poller.shell_calls.append(cmd) + return None # the transport delivered nothing + return await real_shell(cmd) + + poller.shell = _flaky + rows = await manager.async_refresh_installed_apps() + by_pkg = {row["package"]: row for row in rows} + assert by_pkg["org.xbmc.kodi"]["name"] == "org.xbmc.kodi" # fell back + assert manager.app_label("org.xbmc.kodi") is None # NOT cached + + state["fail_kodi"] = False + manager._installed_apps = None + rows2 = await manager.async_refresh_installed_apps() + by_pkg2 = {row["package"]: row for row in rows2} + assert by_pkg2["org.xbmc.kodi"]["name"] == "Kodi" # recovered + + class TestInstalledAppsSensor: """The dashboard surface: state = count, attribute = the table."""