fix(connection): a label dump that returned nothing is a FAILED fetch (t57 UNIT 0)
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.
This commit is contained in:
1 parent
ca3fdec218
commit
9461d69469
5 files changed
+183
-32
No files matched your search
@@ -36,6 +36,7 @@ from .const import (
|
|||||||
ADB_RECONNECT_BACKOFF_CAP,
|
ADB_RECONNECT_BACKOFF_CAP,
|
||||||
ADB_RECONNECT_BACKOFF_START,
|
ADB_RECONNECT_BACKOFF_START,
|
||||||
ADB_RECONNECT_JITTER,
|
ADB_RECONNECT_JITTER,
|
||||||
|
APP_LABEL_EXIT_MARKER,
|
||||||
APP_LABEL_TTL,
|
APP_LABEL_TTL,
|
||||||
CONF_LOGCAT_EVENTS,
|
CONF_LOGCAT_EVENTS,
|
||||||
LOGCAT_SUPPRESS_WINDOW,
|
LOGCAT_SUPPRESS_WINDOW,
|
||||||
@@ -206,7 +207,11 @@ class ShieldConnectionManager:
|
|||||||
# P5-1: device manifest labels — package → (label, stamp). Long-TTL
|
# P5-1: device manifest labels — package → (label, stamp). Long-TTL
|
||||||
# (APP_LABEL_TTL) cache resolved via `pm dump` on demand, one
|
# (APP_LABEL_TTL) cache resolved via `pm dump` on demand, one
|
||||||
# package per watchdog cycle so a burst never blocks the watchdog.
|
# 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()
|
self._app_label_requests: set[str] = set()
|
||||||
# t55: packages whose label fetch is queued or in flight — a render
|
# t55: packages whose label fetch is queued or in flight — a render
|
||||||
# during the fetch must not enqueue the same package twice.
|
# during the fetch must not enqueue the same package twice.
|
||||||
@@ -578,16 +583,14 @@ class ShieldConnectionManager:
|
|||||||
return None
|
return None
|
||||||
cached = self._app_labels.get(package)
|
cached = self._app_labels.get(package)
|
||||||
if cached is not None:
|
if cached is not None:
|
||||||
label, stamp, resolved = cached
|
label, stamp = cached
|
||||||
if self._clock() - stamp <= APP_LABEL_TTL:
|
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.
|
# found nothing — do not re-enqueue, zero per-heartbeat tax.
|
||||||
# An unresolved stamp is a failed fetch: fall through and
|
# A fetch that did not run is not cached at all (t57 UNIT 0),
|
||||||
# retry rather than serving a miss forever.
|
# so there is no "unresolved" shape to fall through on.
|
||||||
if resolved:
|
return label or None
|
||||||
return label or None
|
self._app_labels.pop(package, None) # stale — refetch on demand
|
||||||
else:
|
|
||||||
self._app_labels.pop(package, None) # stale — refetch on demand
|
|
||||||
if package not in self._app_label_requests and (
|
if package not in self._app_label_requests and (
|
||||||
package not in self._app_label_inflight
|
package not in self._app_label_inflight
|
||||||
):
|
):
|
||||||
@@ -595,15 +598,15 @@ class ShieldConnectionManager:
|
|||||||
return None
|
return None
|
||||||
|
|
||||||
def cache_app_label(self, package: str, label: str | None) -> 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 only writer. ``None``/"" is the negative result devices
|
||||||
the cache's shape stays private: a caller (tests, or a future
|
genuinely report for label-less packages, and it is definitive: no
|
||||||
discovery path that read a label out of band) records that a fetch
|
further ``pm dump`` for ``APP_LABEL_TTL``. A fetch that did NOT run
|
||||||
RAN, whether or not it produced a label. ``None``/"" is the negative
|
must not call this — that was the live defect (see
|
||||||
result devices genuinely report for label-less packages.
|
``_fetch_app_label``).
|
||||||
"""
|
"""
|
||||||
self._app_labels[package] = (label or "", self._clock(), True)
|
self._app_labels[package] = (label or "", self._clock())
|
||||||
|
|
||||||
@property
|
@property
|
||||||
def app_label_requests(self) -> set[str]:
|
def app_label_requests(self) -> set[str]:
|
||||||
@@ -634,18 +637,29 @@ class ShieldConnectionManager:
|
|||||||
_LOGGER.debug("protocol device info unavailable: %s", err)
|
_LOGGER.debug("protocol device info unavailable: %s", err)
|
||||||
|
|
||||||
async def _fetch_app_label(self, package: str) -> None:
|
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
|
* the dump ran and carries none → cache the empty string as a
|
||||||
definitive negative (the t24 F1 rule: no per-heartbeat round-trip
|
definitive negative (t24 F1: no per-heartbeat round-trip tax for a
|
||||||
tax for a label-less package);
|
label-less package);
|
||||||
* the fetch could not run at all (no connected poller, shell error) →
|
* the fetch did NOT run → cache NOTHING, and the next render
|
||||||
cache NOTHING, so the next render re-enqueues and the miss is
|
re-enqueues. ``ShieldAdbPoller.shell`` answers ``None`` for both
|
||||||
retried instead of being silent forever. Failures are logged at
|
shapes of "did not run" — no connected device, and a command that
|
||||||
debug so ``app_name`` falling back to a package id is diagnosable.
|
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)
|
self._app_label_inflight.add(package)
|
||||||
try:
|
try:
|
||||||
@@ -661,9 +675,9 @@ class ShieldConnectionManager:
|
|||||||
import shlex
|
import shlex
|
||||||
|
|
||||||
dump = await poller.shell(
|
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:
|
except Exception as err:
|
||||||
_LOGGER.debug(
|
_LOGGER.debug(
|
||||||
"app label fetch failed for %s (%s) — will retry on the "
|
"app label fetch failed for %s (%s) — will retry on the "
|
||||||
@@ -672,9 +686,18 @@ class ShieldConnectionManager:
|
|||||||
err,
|
err,
|
||||||
)
|
)
|
||||||
return
|
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
|
# F1: label-less pm dumps negatively cache too (empty label) so a
|
||||||
# later cycle performs zero additional pm dumps for the package.
|
# 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:
|
finally:
|
||||||
self._app_label_inflight.discard(package)
|
self._app_label_inflight.discard(package)
|
||||||
def _on_logcat_app_start(self, package: str) -> None:
|
def _on_logcat_app_start(self, package: str) -> None:
|
||||||
|
|||||||
@@ -121,6 +121,13 @@ CONF_APP_ICON_MAP = "app_icon_map"
|
|||||||
# updates, not per poll).
|
# updates, not per poll).
|
||||||
APP_LABEL_TTL = 24 * 3600
|
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
|
# P4-2a adb_command service: rawADB shell on this entry — OFF by default
|
||||||
# (same security posture as the core androidtv integration).
|
# (same security posture as the core androidtv integration).
|
||||||
CONF_ADB_COMMAND_ENABLED = "adb_command_enabled"
|
CONF_ADB_COMMAND_ENABLED = "adb_command_enabled"
|
||||||
|
|||||||
@@ -55,7 +55,6 @@ def running_apps_from_tasks(
|
|||||||
app["state"] = "buffering"
|
app["state"] = "buffering"
|
||||||
elif session.paused:
|
elif session.paused:
|
||||||
app["state"] = "paused"
|
app["state"] = "paused"
|
||||||
app["state"] = "paused"
|
|
||||||
else:
|
else:
|
||||||
app["state"] = "idle"
|
app["state"] = "idle"
|
||||||
if session.title:
|
if session.title:
|
||||||
|
|||||||
@@ -16,7 +16,7 @@ import pytest
|
|||||||
from conftest import _SHIELD_DIR
|
from conftest import _SHIELD_DIR
|
||||||
from entity_stublib import install, load_entity_modules
|
from entity_stublib import install, load_entity_modules
|
||||||
from nsh.connection import ShieldConnectionManager
|
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)
|
@pytest.fixture(autouse=True)
|
||||||
@@ -1619,6 +1619,8 @@ class _LabelPoller(FakeAdbPoller):
|
|||||||
super().__init__()
|
super().__init__()
|
||||||
self.pm_dumps: dict[str, str] = {}
|
self.pm_dumps: dict[str, str] = {}
|
||||||
self.dumps: list[str] = []
|
self.dumps: list[str] = []
|
||||||
|
#: t57: True = the transport never delivered output (a FAILED fetch)
|
||||||
|
self.shell_failure = False
|
||||||
|
|
||||||
@property
|
@property
|
||||||
def connected(self):
|
def connected(self):
|
||||||
@@ -1644,12 +1646,25 @@ class _LabelPoller(FakeAdbPoller):
|
|||||||
return []
|
return []
|
||||||
|
|
||||||
async def shell(self, cmd: str) -> str | None:
|
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)
|
self.dumps.append(cmd)
|
||||||
import shlex
|
import shlex
|
||||||
|
|
||||||
|
if self.shell_failure:
|
||||||
|
return None
|
||||||
for package, dump in self.pm_dumps.items():
|
for package, dump in self.pm_dumps.items():
|
||||||
if shlex.quote(package) in cmd or package in cmd:
|
if shlex.quote(package) in cmd or package in cmd:
|
||||||
return dump
|
return dump
|
||||||
|
if APP_LABEL_EXIT_MARKER in cmd:
|
||||||
|
return f"{APP_LABEL_EXIT_MARKER}0\n"
|
||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
||||||
@@ -1753,6 +1768,110 @@ class TestCurrentAppNameEndToEndT55:
|
|||||||
assert sensor.native_value == "com.example.unknown"
|
assert sensor.native_value == "com.example.unknown"
|
||||||
assert "com.example.unknown" in manager._app_labels
|
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):
|
async def test_the_fetch_targets_the_app_on_screen_not_the_previous_one(self, modules):
|
||||||
player, manager, sensor, poller = self._wire(
|
player, manager, sensor, poller = self._wire(
|
||||||
modules,
|
modules,
|
||||||
|
|||||||
@@ -18,7 +18,7 @@ from nsh.connection import (
|
|||||||
ShieldConnectionManager,
|
ShieldConnectionManager,
|
||||||
StateStore,
|
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():
|
def test_connection_module_depends_on_no_homeassistant():
|
||||||
@@ -310,8 +310,11 @@ class TestAppLabelCache:
|
|||||||
|
|
||||||
await asyncio.sleep(0) # drain the fetch task
|
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 == [
|
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}$?"
|
||||||
]
|
]
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user