fix(transport): no stale protocol error, no connect task outliving its entry, honest banners (t20)

The t19 review's findings on the protocol transport, all of them closed here.

O1 (MEDIUM, FIRST). `_note_protocol_state(PROTOCOL_CONNECTED, None)` only assigned
`last_error` when its argument was non-None, so a failure kept being published
after the transport recovered: the review measured
`protocol_last_error == "connect: Net: boom"` beside `protocol_connected: true` —
a contradiction on the exact attributes the diagnostic surface was built for, so
the user could not answer "is the protocol working?" without a device test.

The pair now has ONE stated meaning, in the code that owns it and in the remote's
attribute docs: `protocol_state`/`protocol_connected` is the transport's CURRENT
condition, and `protocol_last_error` is the failure the current state has NOT
recovered from. A session becoming proven CLEARS it — and a session becomes proven
in BOTH places that can prove one, so the connect success and the transport's own
availability push now take the same route (the push previously assigned the state
directly and would have left the stale error again). `protocol_failures` keeps
counting, so a recovery stays visible as history rather than as a stale message.
A failure that happens AFTER the session came up is still kept: that one is
current, not stale, and a test pins that distinction so a future "just clear it
everywhere" refactor cannot make failures invisible.

O4 + O5 (the lifecycle leaks, now real tests instead of observations). The connect
attempt was never in the manager's task collection, so `async_stop` never cancelled
it — a reconnect loop could outlive its entry, and since every options save reloads
the entry those loops could accumulate across saves. `async_stop` also only
SCHEDULED its cancellations. The attempt is now tracked, `async_stop` cancels AND
awaits every tracked task, and the stop path refuses to start anything new. A
double start is deterministic instead of orphaning the first: a live attempt is
reused, an already-handed-over session (connected or disconnected — the library's
keep_reconnecting owns it) starts nothing, and only a FINISHED attempt that never
got a session may be replaced. The guard sits before the push callbacks are
registered, so a second start cannot double-register them either.

O2 (LOW). The connect banner claimed liveliness a successful `async_connect()`
cannot know. It now says exactly what it proved (the client accepted a session)
and hands liveness to what can actually report it (the transport's own
availability push). STALE is defined where the fixes live: a banner is stale when a
later state change never followed it up, so the log's last word contradicts the
entity. Every transition therefore announces itself with both states — INFO for
the two a user acts on, DEBUG otherwise — which is what makes the last transport
line always match the published state.

O3 (LOW). The drift floors now carry an explicit MARGIN POLICY instead of leaving
a 2-margin unexplained: a floor is a vacuity-and-blindness alarm, not a size
ratchet (it must fail when the walk stops COVERING something, not when a
legitimate change removes code). Owner-counters keep a 2 margin (the project's own
t62 canary, now stated once), statement_blocks moves to a ~2% band (28 blocks —
the t19 complaint was that a 23-statement refactor could trip it), relative_imports
~2.5%, and the awaited-calls floor is called out plainly as the weakest one because
a percentage margin on six sites is meaningless. The rule for changing any floor is
written down with it. The walk is now 1648 blocks, so the floor is re-derived:
1615.

EVIDENCE THAT THE TESTS PIN THE FIX (not merely pass): the same test file was run
against the PRE-fix source (`git archive HEAD`, a throwaway copy under /tmp) and 13
of the 15 new tests FAIL there. The two that pass pre-fix are the deliberate
anti-overcorrection pins: a failure that is still current keeps its message, and a
terminal (auth-rejected) attempt may still be replaced.

No regressions: 889 passed / 5 skipped, 890 / 4 with androidtvremote2, the real-HA
pattern (3 files) 74 passed with no skips, ruff clean, drift exit 0.

NOT VERIFIED HERE: no real Shield and no paired device, so no real TLS session,
certificate or push was ever received — the connect path runs against doubles plus
the installed library's real source, and the *timing* of a cancellation during a
live handshake is inferred from the awaited-cancel path, not observed on hardware.

custom_components/nvidia_shield/connection.py

@@ -330,6 +330,10 @@ class ShieldConnectionManager:
             PROTOCOL_NEVER_CONNECTED if self._protocol is not None else PROTOCOL_OFF
         )
         self._protocol_last_error: str | None = None
+        # t20/O5: the connect ATTEMPT's task. Held so a second start recognises a
+        # live attempt instead of orphaning it; it is also tracked in ``_tasks`` so
+        # ``async_stop`` cancels it (t20/O4).
+        self._protocol_connect_task: asyncio.Task | None = None
         self._protocol_ever_authenticated = False
         self._protocol_failures = 0
 
@@ -403,6 +407,8 @@ class ShieldConnectionManager:
         watchdog is built here. Push callbacks write straight into the
         StateStore and ping registered listeners.
         """
+        if self._stopping:
+            return  # a stopped manager must not restart itself (t20/O4/O5)
         if self._protocol is None:
             if self._protocol_factory is None:
                 if self._protocol_settings is None:
@@ -412,6 +418,27 @@ class ShieldConnectionManager:
         client = self._protocol
         if client is None:
             return
+        # t20/O5: A SECOND START IS DETERMINISTIC. A live attempt is reused and an
+        # established session is left alone, so nothing is ever orphaned — and the
+        # guard sits BEFORE the push callbacks below, so a second call cannot
+        # register them twice either (which would double every device push).
+        attempt = self._protocol_connect_task
+        if attempt is not None and not attempt.done():
+            _LOGGER.debug(
+                "Shield remote-protocol connect attempt already in flight — reusing it"
+            )
+            return
+        if self._protocol_state in (PROTOCOL_CONNECTED, PROTOCOL_DISCONNECTED):
+            # A session exists (or existed and is being maintained): the LIBRARY's own
+            # keep-reconnecting loop owns it from here, and calling keep_reconnecting
+            # a second time would put a second loop over the same session — so a
+            # second start never builds one.
+            _LOGGER.debug(
+                "Shield remote-protocol session already handed to the library's "
+                "keep-reconnecting loop (state: %s) — no second attempt",
+                self._protocol_state,
+            )
+            return
         # Real client: make sure local certificate material exists before it
         # connects (fails soft — pair flows can regenerate).
         generate_cert = getattr(client, "async_generate_cert_if_missing", None)
@@ -444,6 +471,11 @@ class ShieldConnectionManager:
         self._protocol_connect_task = self._create_task(
             self._protocol_connect_loop(client), "shield_protocol_connect"
         )
+        # t20/O4: TRACKED. Cancelling only the tasks the manager holds is what makes
+        # an unload complete: an untracked connect attempt kept retrying after its
+        # entry was gone, and since every options save reloads the entry those loops
+        # could accumulate across saves.
+        self._tasks.append(self._protocol_connect_task)
 
     async def _protocol_connect_loop(self, client) -> None:
         """Open the protocol session, then let the library keep it open (t15).
@@ -489,9 +521,19 @@ class ShieldConnectionManager:
                 # Connected: remember it as an authenticated session and hand the
                 # socket to the library's own reconnect task (one owner per
                 # transport — the stop path closes the client, which tears it down).
+                # t20/O2: the banner says only what THIS attempt proved. A
+                # successful async_connect() cannot know the device stays reachable,
+                # so it must not claim liveliness — that is the transport's own
+                # business from here (its availability push and the library's
+                # keep-reconnecting loop). The state announcement that follows is the
+                # log's last word on the transport, and it is the state the entity
+                # publishes: that is what keeps the two from contradicting.
                 _LOGGER.info(
-                    "Shield remote-protocol transport connected to %s",
+                    "Shield remote-protocol session opened on %s — what this proves: "
+                    "the client accepted a session; what maintains it from here: the "
+                    "transport's own availability push (state: %s)",
                     getattr(client, "host", self.entry.title if self.entry else "?"),
+                    PROTOCOL_CONNECTED,
                 )
                 self._protocol_ever_authenticated = True
                 self._note_protocol_state(PROTOCOL_CONNECTED, None)
@@ -555,8 +597,11 @@ class ShieldConnectionManager:
         """
         self._protocol_ever_authenticated = True
         if field == FIELD_IS_AVAILABLE:
-            self._protocol_state = (
-                PROTOCOL_CONNECTED if value else PROTOCOL_DISCONNECTED
+            # t20/O1: the transport's own availability report is the OTHER place a
+            # session becomes provable, so it takes the same route — a healthy push
+            # clears a recovered failure exactly as a successful connect does.
+            self._apply_protocol_state(
+                PROTOCOL_CONNECTED if value else PROTOCOL_DISCONNECTED, None
             )
         self.store.set(field, value)
         loop = self._protocol_loop
@@ -645,13 +690,78 @@ class ShieldConnectionManager:
         return self._protocol_failures
 
 
-    def _note_protocol_state(self, state: str, error: str | None) -> None:
-        """Record the transport state (and an error when there is one) + notify."""
-        changed = state != self._protocol_state
+    def _apply_protocol_state(self, state: str, error: str | None) -> bool:
+        """Set the published pair and announce a TRANSITION; True if it changed.
+
+        One place decides what the pair MEANS (``_note_protocol_state`` documents
+        it) and one place decides when a change is announced, so a connect attempt
+        and a device push cannot publish two different stories.
+        """
+        previous = self._protocol_state
+        state_changed = state != previous
         self._protocol_state = state
-        if error is not None:
+        error_changed = False
+        if state == PROTOCOL_CONNECTED:
+            # t20/O1: a proven session ENDS the failure episode. The recovery stays
+            # visible in ``protocol_failures`` (history), never as a stale message
+            # published next to ``protocol_connected: true``.
+            error_changed = self._forget_protocol_error()
+        elif error is not None:
+            error_changed = error != self._protocol_last_error
             self._protocol_last_error = error
-        if changed:
+        if state_changed:
+            self._announce_protocol_state(previous, state)
+        return state_changed or error_changed
+
+    def _forget_protocol_error(self) -> bool:
+        """Drop a failure the transport has since recovered from (t20/O1).
+
+        Returns True when there WAS one, so the caller knows listeners must be
+        told that a published attribute changed.
+        """
+        if self._protocol_last_error is None:
+            return False
+        self._protocol_last_error = None
+        return True
+
+    def _announce_protocol_state(self, previous: str, state: str) -> None:
+        """Announce a transport TRANSITION — this is what keeps banners honest.
+
+        t20/O2 defines a STALE banner as one whose transport claim later state
+        changes never followed up on: the log's last word would then contradict what
+        the entity publishes. Every transition therefore logs one line naming both
+        states, at INFO for the two states a user acts on (a session came up, a
+        session went down) and at DEBUG otherwise (``never_connected`` repeats per
+        retry, and the auth rejection is already logged with its reason by
+        ``_on_invalid_auth``).
+        """
+        _LOGGER.log(
+            logging.INFO
+            if state in (PROTOCOL_CONNECTED, PROTOCOL_DISCONNECTED)
+            else logging.DEBUG,
+            "Shield remote-protocol transport state: %s → %s",
+            previous,
+            state,
+        )
+
+    def _note_protocol_state(self, state: str, error: str | None) -> None:
+        """Record the transport state (and an error when there is one) + notify.
+
+        WHAT THE PUBLISHED PAIR MEANS (t20/O1): ``protocol_state`` — and its
+        boolean twin ``protocol_connected`` — is the transport's CURRENT condition,
+        while ``protocol_last_error`` is the failure the current state has NOT
+        recovered from. A session becoming proven (a successful connect, or the
+        transport's own availability push) therefore CLEARS it, so an error can
+        never be published beside a healthy ``protocol_connected``. A failure that
+        happens AFTER the session came up is kept — that one is current, not stale.
+        Before this, ``_note_protocol_state(PROTOCOL_CONNECTED, None)`` left the
+        previous failure in place and the entity published ``protocol_last_error ==
+        "connect: Net: boom"`` beside ``protocol_connected: true`` — on the very
+        attributes a user is told to read to answer "is the protocol working?"
+        (t19 review, O1). ``protocol_failures`` still counts, so a recovery remains
+        visible as history instead of as a stale message.
+        """
+        if self._apply_protocol_state(state, error):
             self._notify()
 
     def _note_protocol_send_failure(self, what: str, err) -> None:
@@ -1802,6 +1912,12 @@ class ShieldConnectionManager:
         tasks, self._tasks = self._tasks, []
         for task in tasks:
             task.cancel()
+        # t20/O4: cancel() only SCHEDULES the cancellation, and an unload that
+        # returns while a connect attempt is still running is exactly how a loop
+        # outlived its entry. Await them: their exceptions (CancelledError included)
+        # are results here, not failures — being cancelled IS how a manager task ends.
+        if tasks:
+            await asyncio.gather(*tasks, return_exceptions=True)
         self._listeners.clear()
         self._last_known.clear()
         self._app_labels.clear()

custom_components/nvidia_shield/remote.py

@@ -238,7 +238,14 @@ class NvidiaShieldRemoteEntity(RemoteEntity):
         * ``protocol_current_app_seen`` / ``protocol_is_on_seen`` /
           ``protocol_volume_seen`` — whether each device push has EVER arrived.
         * ``protocol_last_error`` / ``protocol_failures`` — a send that could not
-          run is recorded here and logged, never swallowed into a debug line.
+          run is recorded here and logged, never swallowed into a debug line. The
+          ERROR is the failure the CURRENT state has not recovered from (t20/O1):
+          it is cleared as soon as a session is proven again — the next connect or
+          the transport's own availability push — so it can never read as a
+          problem beside ``protocol_connected: true``, and a failure that happened
+          AFTER the session came up is kept because that one is still current.
+          ``protocol_failures`` keeps counting, so a recovery stays visible as
+          history instead of as a stale message.
         * ``protocol_certificate`` — the setup pairing's shared ``client.pem`` is
           still on disk, which is exactly why enabling the transport asks for no
           new PIN (the other Android TV integration pairs its own client).