fix(discovery): a cached negative is an answer, not a miss (t57)
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.
This commit is contained in:
1 parent
364d4b7da3
commit
ff8dd1c454
2 files changed
+102
-5
No files matched your search
@@ -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
|
||||
|
||||
|
||||
@@ -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."""
|
||||
|
||||
|
||||
Reference in new issue
Block a user