From 9461d694698b18f5635016f178614c639ec46813 Mon Sep 17 00:00:00 2001 From: James Mackay Date: Mon, 12 Oct 2026 07:21:34 +0100 Subject: [PATCH] fix(connection): a label dump that returned nothing is a FAILED fetch (t57 UNIT 0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit t59 F1 (HIGH). `ShieldAdbPoller.shell` swallows its error and answers None (adb_client.py:144-155), and `_fetch_app_label` read that as "the dump ran and found no label" → `cache_app_label(pkg, None)` → `('', stamp, resolved=True)`. That is a DEFINITIVE negative for APP_LABEL_TTL = 24 h, so after ONE transient failure `app_label()` never re-enqueued, exactly one `pm dump` was ever attempted, and a later good dump could not recover the label — `sensor.native_value` and `media_player.app_name` stayed on the package id for every later render. The dump now carries an exit marker (`; echo __NSH_EXIT:$?`), so the three outcomes are distinguishable: * output with an `application-label` → cache the label; * output that ran and carried none (the marker is still there) → cache the empty string, the definitive negative that keeps t24 F1's zero-round-trip rule for a genuinely label-less package; * no output at all (`None`, or blank) → cache NOTHING and let the next render re-enqueue, logged at debug. The `resolved` flag goes with it: `cache_app_label` was its only writer and always passed True, so an entry in `_app_labels` is now simply "a fetch RAN" and the dead third field (`(label, stamp, True)`) is gone. Tests drive the FakePoller-returns-None AND returns-'' shapes through the sensor and through `media_player.app_name`, plus the label-less package that must still be negatively cached. The stub poller now models both real outcomes of a dump (an answer, or a transport that delivered nothing) instead of conflating them — which is why the old tests could only pin the raising-shell branch that already worked. 4 of the new tests fail on the pre-fix tree, showing `('', stamp, True)` stamped. Also in this commit: - t59 F3: running_apps.py set `app["state"] = "paused"` twice — one line. - t59 F4: no captured fixture contains state=6, so the "the Shield's own sessions report 6 like stock Android" wording in README and the media_session/media_player docstrings is reworded as INFERRED from the `state=PlaybackState {state=N, …}` line shape. A `git notes` entry on 07682ff records the same correction against the t55 commit body. --- custom_components/nvidia_shield/connection.py | 79 ++++++++---- custom_components/nvidia_shield/const.py | 7 + .../ha_adb_common/running_apps.py | 1 - tests/test_command_routing.py | 121 +++++++++++++++++- tests/test_connection.py | 7 +- 5 files changed, 183 insertions(+), 32 deletions(-) diff --git a/custom_components/nvidia_shield/connection.py b/custom_components/nvidia_shield/connection.py index e95b6d0..478c315 100644 --- a/custom_components/nvidia_shield/connection.py +++ b/custom_components/nvidia_shield/connection.py @@ -36,6 +36,7 @@ from .const import ( ADB_RECONNECT_BACKOFF_CAP, ADB_RECONNECT_BACKOFF_START, ADB_RECONNECT_JITTER, + APP_LABEL_EXIT_MARKER, APP_LABEL_TTL, CONF_LOGCAT_EVENTS, LOGCAT_SUPPRESS_WINDOW, @@ -206,7 +207,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, bool]] = {} + # An entry means "a fetch RAN" (t57 UNIT 0): a tuple whose label is + # "" is a definitive negative, and a fetch that could not run is not + # in here at all — so the flag t55 carried to say so was dead weight + # (its only writer always passed True) and is gone. + self._app_labels: dict[str, tuple[str, float]] = {} 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. @@ -578,16 +583,14 @@ class ShieldConnectionManager: return None cached = self._app_labels.get(package) if cached is not None: - label, stamp, resolved = cached + label, stamp = cached if self._clock() - stamp <= APP_LABEL_TTL: - # empty label = negative cache (t24 F1): the pm dump ran and + # empty label = negative cache (t24 F1): the pm dump RAN and # 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 + # A fetch that did not run is not cached at all (t57 UNIT 0), + # so there is no "unresolved" shape to fall through on. + return label or None + 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 ): @@ -595,15 +598,15 @@ class ShieldConnectionManager: return None def cache_app_label(self, package: str, label: str | None) -> None: - """Seed the label cache with a definitive result (t55). + """Record a label fetch that RAN, and what it found (t55, t57). - 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. + The cache's only writer. ``None``/"" is the negative result devices + genuinely report for label-less packages, and it is definitive: no + further ``pm dump`` for ``APP_LABEL_TTL``. A fetch that did NOT run + must not call this — that was the live defect (see + ``_fetch_app_label``). """ - self._app_labels[package] = (label or "", self._clock(), True) + self._app_labels[package] = (label or "", self._clock()) @property def app_label_requests(self) -> set[str]: @@ -634,18 +637,29 @@ class ShieldConnectionManager: _LOGGER.debug("protocol device info unavailable: %s", err) async def _fetch_app_label(self, package: str) -> None: - """One `pm dump` label fetch; stamps ONLY a fetch that really ran (t55). + """One `pm dump` label fetch; stamps ONLY a fetch that really RAN (t57). - Three outcomes, three postures: + The distinction this method exists for (t59 F1): a FAILED fetch and a + genuinely label-less package are not the same thing, and conflating + them is what kept ``app_name`` on the raw package id forever. - * the dump ran and carries an `application-label` → cache the label; + * 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. + definitive negative (t24 F1: no per-heartbeat round-trip tax for a + label-less package); + * the fetch did NOT run → cache NOTHING, and the next render + re-enqueues. ``ShieldAdbPoller.shell`` answers ``None`` for both + shapes of "did not run" — no connected device, and a command that + raised — so ``None``/blank output is treated as a failure rather + than being read as "the dump ran and found no label". Treating it + as the latter stamped a 24 h definitive negative after ONE transient + failure, and a later good dump could never recover the label. + + The dump also carries an exit marker (``; echo __NSH_EXIT:$?``), which + is what keeps the third branch from swallowing a REAL empty answer: if + the command ran and produced nothing, the marker is still there, so + the negative cache happens (zero further round trips). If the output + is blank the command never delivered anything, and we retry. """ self._app_label_inflight.add(package) try: @@ -661,9 +675,9 @@ class ShieldConnectionManager: import shlex dump = await poller.shell( - f"pm dump {shlex.quote(package)} 2>/dev/null | head -300" + f"pm dump {shlex.quote(package)} 2>/dev/null | head -300; " + f"echo {APP_LABEL_EXIT_MARKER}$?" ) - label = parse_application_label(dump or "") except Exception as err: _LOGGER.debug( "app label fetch failed for %s (%s) — will retry on the " @@ -672,9 +686,18 @@ class ShieldConnectionManager: err, ) return + if dump is None or not dump.strip(): + # the transport never delivered output: a FAILED fetch, not an + # answer. Caching here is the t55 defect this repairs. + _LOGGER.debug( + "app label fetch for %s returned no output — will retry on " + "the next render", + package, + ) + 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) + self.cache_app_label(package, parse_application_label(dump)) finally: self._app_label_inflight.discard(package) def _on_logcat_app_start(self, package: str) -> None: diff --git a/custom_components/nvidia_shield/const.py b/custom_components/nvidia_shield/const.py index 119d719..3833564 100644 --- a/custom_components/nvidia_shield/const.py +++ b/custom_components/nvidia_shield/const.py @@ -121,6 +121,13 @@ CONF_APP_ICON_MAP = "app_icon_map" # updates, not per poll). APP_LABEL_TTL = 24 * 3600 +# t57 UNIT 0: exit marker appended to the label dump so a FAILED fetch can be +# told from a package that genuinely has no label. ``adb shell`` output ending +# in this marker means the command ran; an empty answer still carries it, and +# a transport that never delivered output does not. The value is arbitrary but +# unlikely to collide with a manifest line. +APP_LABEL_EXIT_MARKER = "__NSH_EXIT:" + # P4-2a adb_command service: rawADB shell on this entry — OFF by default # (same security posture as the core androidtv integration). CONF_ADB_COMMAND_ENABLED = "adb_command_enabled" diff --git a/custom_components/nvidia_shield/ha_adb_common/running_apps.py b/custom_components/nvidia_shield/ha_adb_common/running_apps.py index aaf4e3b..18d95f0 100644 --- a/custom_components/nvidia_shield/ha_adb_common/running_apps.py +++ b/custom_components/nvidia_shield/ha_adb_common/running_apps.py @@ -55,7 +55,6 @@ def running_apps_from_tasks( app["state"] = "buffering" elif session.paused: app["state"] = "paused" - app["state"] = "paused" else: app["state"] = "idle" if session.title: diff --git a/tests/test_command_routing.py b/tests/test_command_routing.py index 2ead4e8..757d02f 100644 --- a/tests/test_command_routing.py +++ b/tests/test_command_routing.py @@ -16,7 +16,7 @@ import pytest from conftest import _SHIELD_DIR from entity_stublib import install, load_entity_modules from nsh.connection import ShieldConnectionManager -from nsh.const import TIVIMATE_PACKAGE +from nsh.const import APP_LABEL_EXIT_MARKER, TIVIMATE_PACKAGE @pytest.fixture(autouse=True) @@ -1619,6 +1619,8 @@ class _LabelPoller(FakeAdbPoller): super().__init__() self.pm_dumps: dict[str, str] = {} self.dumps: list[str] = [] + #: t57: True = the transport never delivered output (a FAILED fetch) + self.shell_failure = False @property def connected(self): @@ -1644,12 +1646,25 @@ class _LabelPoller(FakeAdbPoller): return [] async def shell(self, cmd: str) -> str | None: + """Model BOTH real outcomes of a label dump (t57). + + A package in ``pm_dumps`` answers with its dump. A package that is NOT + there answers with the exit marker and nothing else — the shape a real + transport produces for "the command ran and this package has no + label". ``shell_failure`` turns every dump into the OTHER real shape: + the transport never delivered output at all (``adb_client.shell`` + returns None when it is not connected or the command raised). + """ self.dumps.append(cmd) import shlex + if self.shell_failure: + return None for package, dump in self.pm_dumps.items(): if shlex.quote(package) in cmd or package in cmd: return dump + if APP_LABEL_EXIT_MARKER in cmd: + return f"{APP_LABEL_EXIT_MARKER}0\n" return None @@ -1753,6 +1768,110 @@ class TestCurrentAppNameEndToEndT55: assert sensor.native_value == "com.example.unknown" assert "com.example.unknown" in manager._app_labels + @pytest.mark.parametrize("blank", [None, ""], ids=["none", "empty"]) + async def test_a_dump_with_no_output_is_a_failure_not_an_answer( + self, modules, blank + ): + """t59 F1 (HIGH): the live symptom, driven through the SENSOR. + + ``adb_client.shell`` swallows its error and returns None, and a real + dump can also come back as an empty string. Neither is the device + saying "this package has no label": reading either as an answer stamped + a 24 h definitive negative after ONE transient failure, so the sensor + stayed on the package id, exactly one dump was ever attempted, and a + later GOOD dump could not recover the label. Both shapes are driven + here — the pre-fix tree fails this at the second cycle. + """ + player, manager, sensor, poller = self._wire( + modules, "ar.tvplayer.tv", {"ar.tvplayer.tv": "application-label: 'TiviMate'"} + ) + + async def _blank(_cmd): + poller.dumps.append(_cmd) + return blank + + # cycle 1: the transport delivered nothing → a FAILED fetch + poller.shell = _blank + assert sensor.native_value == "ar.tvplayer.tv" + await manager._watchdog_cycle(5.0) + await asyncio.sleep(0) + + assert "ar.tvplayer.tv" not in manager._app_labels # nothing stamped + assert sensor.native_value == "ar.tvplayer.tv" + + # the next render re-enqueues, and the retry is what recovers the label + assert manager.app_label("ar.tvplayer.tv") is None + assert manager.app_label_requests == {"ar.tvplayer.tv"} + dump_count = len([d for d in poller.dumps if "pm dump" in d]) + + real_shell = _LabelPoller.shell.__get__(poller, _LabelPoller) + poller.shell = real_shell + await manager._watchdog_cycle(5.0) + await asyncio.sleep(0) + + # a later GOOD dump recovers it — the pre-fix cache made this impossible + assert len([d for d in poller.dumps if "pm dump" in d]) == dump_count + 1 + assert sensor.native_value == "TiviMate" + assert sensor.extra_state_attributes["app_name"] == "TiviMate" + assert player.app_name == "TiviMate" + + @pytest.mark.parametrize("blank", [None, ""], ids=["none", "empty"]) + async def test_the_player_recovers_the_label_after_a_failed_fetch( + self, modules, blank + ): + """Same defect, driven through ``media_player.app_name`` (t59 F1). + + The sensor and the player read the same chain, but they are two + different surfaces and the reviewer's reproduction observed BOTH + stuck on the package id — so both are pinned. + """ + player, manager, sensor, poller = self._wire( + modules, "com.netflix.ninja", {"com.netflix.ninja": "application-label: 'Netflix'"} + ) + + async def _blank(_cmd): + poller.dumps.append(_cmd) + return blank + + poller.shell = _blank + assert player.app_name == "com.netflix.ninja" + await manager._watchdog_cycle(5.0) + await asyncio.sleep(0) + assert "com.netflix.ninja" not in manager._app_labels + assert player.app_name == "com.netflix.ninja" + + assert manager.app_label("com.netflix.ninja") is None # re-enqueued + poller.shell = _LabelPoller.shell.__get__(poller, _LabelPoller) + await manager._watchdog_cycle(5.0) + await asyncio.sleep(0) + + assert player.app_name == "Netflix" + assert sensor.native_value == "Netflix" + + async def test_a_genuinely_label_less_package_is_still_negatively_cached( + self, modules + ): + """The other half: a dump that RAN and found no label stays definitive. + + This is what keeps the t24 F1 rule closed — a label-less package costs + zero further round trips. The marker in the output is what separates + this from the failed fetch above. + """ + player, manager, sensor, poller = self._wire(modules, "com.oem.blank", {}) + + assert sensor.native_value == "com.oem.blank" + await manager._watchdog_cycle(5.0) + await asyncio.sleep(0) + + assert "com.oem.blank" in manager._app_labels # a definitive negative + assert manager.app_label("com.oem.blank") is None + assert manager.app_label_requests == set() # no re-enqueue, ever + + dumps = [d for d in poller.dumps if "pm dump" in d] + await manager._watchdog_cycle(5.0) + await asyncio.sleep(0) + assert [d for d in poller.dumps if "pm dump" in d] == dumps # zero more + async def test_the_fetch_targets_the_app_on_screen_not_the_previous_one(self, modules): player, manager, sensor, poller = self._wire( modules, diff --git a/tests/test_connection.py b/tests/test_connection.py index c8db215..8856873 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -18,7 +18,7 @@ from nsh.connection import ( ShieldConnectionManager, StateStore, ) -from nsh.const import ADB_POLL_INTERVAL +from nsh.const import ADB_POLL_INTERVAL, APP_LABEL_EXIT_MARKER def test_connection_module_depends_on_no_homeassistant(): @@ -310,8 +310,11 @@ class TestAppLabelCache: await asyncio.sleep(0) # drain the fetch task + # t57: the dump carries the exit marker too, so a blank answer can be + # told from a FAILED fetch; the package is still quoted assert manager._poller.shell_calls == [ - f"pm dump {shlex.quote(wicked)} 2>/dev/null | head -300" + f"pm dump {shlex.quote(wicked)} 2>/dev/null | head -300; " + f"echo {APP_LABEL_EXIT_MARKER}$?" ]