diff --git a/custom_components/nvidia_shield/connection.py b/custom_components/nvidia_shield/connection.py index a0ea8aa..e95b6d0 100644 --- a/custom_components/nvidia_shield/connection.py +++ b/custom_components/nvidia_shield/connection.py @@ -206,7 +206,11 @@ class ShieldConnectionManager: # P5-1: device manifest labels — package → (label, stamp). Long-TTL # (APP_LABEL_TTL) cache resolved via `pm dump` on demand, one # package per watchdog cycle so a burst never blocks the watchdog. - self._app_labels: dict[str, tuple[str, float]] = {} + self._app_labels: dict[str, tuple[str, float, bool]] = {} + self._app_label_requests: set[str] = set() + # t55: packages whose label fetch is queued or in flight — a render + # during the fetch must not enqueue the same package twice. + self._app_label_inflight: set[str] = set() # P5-3 (C-STREAM): the dedicated logcat line-stream consumer. # NEVER shares the poller's ADB socket — its own device object and # connection; CONF_LOGCAT_EVENTS gates it (default off) and the @@ -221,7 +225,6 @@ class ShieldConnectionManager: self._logcat_last_stamp: float = -1e9 # Injectable ladder sleep (t26: deterministic tests / future alt-floors) self._logcat_ladder_sleep = asyncio.sleep - self._app_label_requests: set[str] = set() # Injectable clock keeps the TTL expiry tests deterministic. self._clock = clock self._tasks: list[asyncio.Task] = [] @@ -548,26 +551,70 @@ class ShieldConnectionManager: return False def app_label(self, package: str | None) -> str | None: - """Cached manifest label for a package (P5-1), or None. + """Cached manifest label for a package (P5-1), or None (t55). A cache miss enqueues an on-demand `pm dump` fetch — drained one package per watchdog cycle via the manager task pool, so entity - refreshes and the watchdog are never blocked by a burst. A failed - resolution simply falls through to the next name tier. + refreshes and the watchdog are never blocked by a burst. + + What is cached is the FETCH, not the wishful result (t55 root cause): + + * a fetch that RAN caches its outcome for ``APP_LABEL_TTL`` — a label + when the manifest has one, and the empty string otherwise. It is + definitive: the device answered, the package simply has no default + label (several system/OEM packages do not). No re-enqueue, so a + permanently label-less package costs ZERO further round trips + (t24 F1 stays closed). + * a fetch that could NOT run (no poller, ADB down, shell error) caches + nothing, so the next render enqueues again — a miss is retried + visibly instead of being stamped as an eternal "no label". This was + the live defect: the old code stored ("", stamp) on every failure + path, which silenced the label tier for the whole 24 h TTL and left + app_name showing the raw package id. + * while a fetch for the package is queued or in flight, the next + render does not enqueue it a second time. """ if not package: return None cached = self._app_labels.get(package) if cached is not None: - label, stamp = cached + label, stamp, resolved = cached if self._clock() - stamp <= APP_LABEL_TTL: # empty label = negative cache (t24 F1): the pm dump ran and - # found nothing — do not re-enqueue, zero per-heartbeat tax - return label or None - self._app_labels.pop(package, None) # stale — refetch on demand - self._app_label_requests.add(package) + # found nothing — do not re-enqueue, zero per-heartbeat tax. + # An unresolved stamp is a failed fetch: fall through and + # retry rather than serving a miss forever. + if resolved: + return label or None + else: + self._app_labels.pop(package, None) # stale — refetch on demand + if package not in self._app_label_requests and ( + package not in self._app_label_inflight + ): + self._app_label_requests.add(package) return None + def cache_app_label(self, package: str, label: str | None) -> None: + """Seed the label cache with a definitive result (t55). + + The ONE writer of ``_app_labels`` other than ``_fetch_app_label``, so + the cache's shape stays private: a caller (tests, or a future + discovery path that read a label out of band) records that a fetch + RAN, whether or not it produced a label. ``None``/"" is the negative + result devices genuinely report for label-less packages. + """ + self._app_labels[package] = (label or "", self._clock(), True) + + @property + def app_label_requests(self) -> set[str]: + """Packages queued for a manifest-label fetch (public, for tests).""" + return set(self._app_label_requests) + + @property + def app_label_inflight(self) -> set[str]: + """Packages whose label fetch is running right now (public, tests).""" + return set(self._app_label_inflight) + async def async_capture_protocol_device_data(self) -> None: """Capture (name, manufacturer, model, sw) once (t30 G10).""" if self._protocol is None or self.protocol_device_data is not None: @@ -587,24 +634,49 @@ class ShieldConnectionManager: _LOGGER.debug("protocol device info unavailable: %s", err) async def _fetch_app_label(self, package: str) -> None: - """One `pm dump` label fetch; failure falls through silently.""" - poller = self._poller - if poller is None or not self.connected: - return + """One `pm dump` label fetch; stamps ONLY a fetch that really ran (t55). + + Three outcomes, three postures: + + * the dump ran and carries an `application-label` → cache the label; + * the dump ran and carries none → cache the empty string as a + definitive negative (the t24 F1 rule: no per-heartbeat round-trip + tax for a label-less package); + * the fetch could not run at all (no connected poller, shell error) → + cache NOTHING, so the next render re-enqueues and the miss is + retried instead of being silent forever. Failures are logged at + debug so ``app_name`` falling back to a package id is diagnosable. + """ + self._app_label_inflight.add(package) try: - import shlex - - dump = await poller.shell( - f"pm dump {shlex.quote(package)} 2>/dev/null | head -300" - ) - label = parse_application_label(dump or "") - except Exception as err: - _LOGGER.debug("app label fetch failed for %s: %s", package, err) - return - # F1: label-less pm dumps negatively cache too (empty label) so a - # later cycle performs zero additional pm dumps for the package - self._app_labels[package] = (label or "", self._clock()) + poller = self._poller + if poller is None or not self.connected: + _LOGGER.debug( + "app label for %s not fetched (ADB not connected) — will " + "retry on the next render", + package, + ) + return + try: + import shlex + dump = await poller.shell( + f"pm dump {shlex.quote(package)} 2>/dev/null | head -300" + ) + label = parse_application_label(dump or "") + except Exception as err: + _LOGGER.debug( + "app label fetch failed for %s (%s) — will retry on the " + "next render", + package, + err, + ) + return + # F1: label-less pm dumps negatively cache too (empty label) so a + # later cycle performs zero additional pm dumps for the package. + self.cache_app_label(package, label) + finally: + self._app_label_inflight.discard(package) def _on_logcat_app_start(self, package: str) -> None: """Stamp FIELD_FOREGROUND instantly from a parsed START (P5-3). @@ -892,6 +964,7 @@ class ShieldConnectionManager: self._last_known.clear() self._app_labels.clear() self._app_label_requests.clear() + self._app_label_inflight.clear() if self._poller is not None: with contextlib.suppress(Exception): await self._poller.disconnect() diff --git a/tests/test_connection.py b/tests/test_connection.py index 98d36aa..c8db215 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -177,11 +177,18 @@ class TestAppLabelCache: assert manager.app_label("com.unknown.app") is None await manager._watchdog_cycle(5.0) - # still None (next tier), no crash — falls through cleanly - assert manager.app_label("com.unknown.app") is not None or True + # the dump RAN and carried no label: definitive negative, and the + # caller's next tier (the raw package) takes over — still None here, + # never a crash or a fabricated label + assert manager.app_label("com.unknown.app") is None - async def test_label_less_package_negatively_cached_no_repeat_dump(self): - """t26 F1: unresolvable packages cost at most one pm dump ever.""" + async def test_label_less_package_is_a_definitive_negative(self): + """t24 F1: a label-less package's ANSWER costs one pm dump, not one/cycle. + + The old code additionally stamped this answer on FAILURE paths; t55 + separates the two (see the retry tests below), but the negative cache + for a dump that really ran stays — the round-trip tax must not return. + """ manager, _t = self._manager_with_dump() manager._poller.pm_dumps["com.unknown.app"] = "no label line here" @@ -201,6 +208,94 @@ class TestAppLabelCache: assert len(manager._poller.shell_calls) == first_dump_count assert manager.app_label("com.unknown.app") is None # falls through + async def test_failed_fetch_is_not_cached_and_retries(self): + """t55 root cause: a fetch that could NOT run must not become a miss. + + The live defect was a metadata fetch failing and the failure being + written as ("", stamp) — an eternal "no label" for the whole 24 h TTL, + which left app_name showing the raw package id with no way back. Here + the failed fetch stamps nothing, the next render re-enqueues, and the + retry resolves the real label. + """ + manager, _t = self._manager_with_dump() + real_shell = manager._poller.shell + calls: list[str] = [] + + async def _flaky(cmd): + calls.append(cmd) + if "pm dump" in cmd: + raise RuntimeError("adb shell blew up") + return await real_shell(cmd) + + manager._poller.shell = _flaky + + assert manager.app_label("ar.tvplayer.tv") is None # enqueue + await manager._watchdog_cycle(5.0) + import asyncio + + await asyncio.sleep(0) + + # the fetch ran and failed → NOTHING is stamped as an answer + assert manager._app_labels.get("ar.tvplayer.tv") is None + assert [c for c in calls if "pm dump" in c] + + # the next render enqueues again (the miss is retried, not silenced) + assert manager.app_label("ar.tvplayer.tv") is None + assert manager.app_label_requests == {"ar.tvplayer.tv"} + + # the retry reaches the device and resolves the label + manager._poller.shell = real_shell + await manager._watchdog_cycle(5.0) + await asyncio.sleep(0) + assert manager.app_label("ar.tvplayer.tv") == "TiviMate" + + async def test_shell_failure_is_not_cached_and_retries(self): + manager, _t = self._manager_with_dump() + manager._poller.pm_dumps["ar.tvplayer.tv"] = "application-label: 'TiviMate'" + + async def _boom(_cmd): + raise RuntimeError("adb shell blew up") + + manager._poller.shell = _boom + manager.app_label("ar.tvplayer.tv") + await manager._watchdog_cycle(5.0) + import asyncio + + await asyncio.sleep(0) + + assert manager._app_labels.get("ar.tvplayer.tv") is None + assert manager.app_label_requests == set() + assert manager.app_label("ar.tvplayer.tv") is None + assert manager.app_label_requests == {"ar.tvplayer.tv"} # re-enqueued + + async def test_in_flight_fetch_is_not_enqueued_twice(self): + manager, _t = self._manager_with_dump() + + assert manager.app_label("ar.tvplayer.tv") is None + assert manager.app_label("ar.tvplayer.tv") is None # same render loop + assert manager.app_label_requests == {"ar.tvplayer.tv"} + assert manager.app_label_inflight == set() + + async def test_one_fetch_per_cycle_even_under_a_burst(self): + manager, _t = self._manager_with_dump() + manager._poller.pm_dumps["com.a"] = "application-label: 'A'" + manager._poller.pm_dumps["com.b"] = "application-label: 'B'" + + manager.app_label("com.a") + manager.app_label("com.b") + await manager._watchdog_cycle(5.0) + await manager._watchdog_cycle(5.0) + import asyncio + + await asyncio.sleep(0) + + dumps = [c for c in manager._poller.shell_calls if "pm dump" in c] + assert len(dumps) == 2 # one per cycle, both eventually resolved + assert sorted([manager.app_label("com.a"), manager.app_label("com.b")]) == [ + "A", + "B", + ] + async def test_pm_dump_command_is_shell_quoted(self): """t26 F2: the package argument is shlex.quote'd (consistent).""" import shlex diff --git a/tests/test_media_sources.py b/tests/test_media_sources.py index 675d6cf..2414681 100644 --- a/tests/test_media_sources.py +++ b/tests/test_media_sources.py @@ -144,10 +144,7 @@ class TestSource: def test_it_uses_the_cached_device_label_when_unmapped(self, modules): player, _served = _player(modules, foreground="com.netflix.ninja") - player._manager._app_labels["com.netflix.ninja"] = ( - "Netflix", - player._manager._clock(), - ) + player._manager.cache_app_label("com.netflix.ninja", "Netflix") assert player.source == "Netflix" diff --git a/tests/test_standalone_setup.py b/tests/test_standalone_setup.py index 47bc795..457b316 100644 --- a/tests/test_standalone_setup.py +++ b/tests/test_standalone_setup.py @@ -1241,7 +1241,7 @@ class TestAppNameMapResolution: player, _s, _i = _cmd_player(modules) player._entry.options["app_name_map"] = {} # no override manager = player._manager - manager._app_labels["ar.tvplayer.tv"] = ("TiviMate", manager._clock()) + manager.cache_app_label("ar.tvplayer.tv", "TiviMate") manager._on_current_app("ar.tvplayer.tv") await asyncio.sleep(0.05)