fix: bound each addon HA-link probe by the readiness deadline (#2028)

* fix: bound each addon HA-link probe by the readiness deadline

`_probe_addon_ha_link` passed no timeout to `Client` or `call_tool`, so an
attempt inherited FastMCP's Streamable HTTP default of
`httpx.Timeout(30.0, read=300.0)` (client/transports/http.py). The poll loop
only consulted the deadline between attempts, so a listener that accepted the
connection but stalled on initialize or the tool response could hold
`wait_for_addon_ha_link_ready(timeout=180)` for ~300s per attempt — delaying the
fixture failure and its HAOS diagnostics well past the advertised budget, and
making the docstring's ceiling untrue.

Pass the time left on the deadline into each attempt and apply it at all three
layers that can stall independently (connect/initialize via `init_timeout`, the
tool response via `call_tool(timeout=...)`, and the surrounding coroutine via
`asyncio.wait_for`). Cap the inter-attempt sleep the same way so the budget is
exact rather than exact-plus-one-poll.

Reported by Codex on #2025.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* ci: run the e2e suites when the change classifier does not succeed

The classifier is written to fail closed — any error inside the script leaves
`run=true` — but that only covers the script choosing a value. It does not cover
the job itself not succeeding. `if: needs.changes.outputs.run == 'true'` carries
an implicit `success()` on `needs`, so a classifier job that fails (on #2028 it
failed with no steps and no runner assigned) skips the suite, and a skipped
required check reports Success to branch protection. A runner hiccup in a
five-minute filter job could therefore wave code through untested — the exact
hole the classifier comment says the design avoids.

Gate the four suites on `!cancelled() && (needs.changes.result != 'success' ||
needs.changes.outputs.run == 'true')` instead: a classifier that succeeded and
said docs-only still skips (unchanged), a classifier that did not succeed now
runs the suite, and a real cancellation still cancels.

pr.yml's `e2e-validation-gate` had the same hole from the other side: it reads
`needs.changes.outputs.run != 'true'` to detect a docs-only PR, and a failed
classifier leaves that output empty, so the gate passed without judging any
lane. It now requires the classifier to have succeeded before honoring the skip
and otherwise falls through to the lane results.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix: retry stalled link probes and classify their teardown errors

Review follow-ups on the per-attempt budget, all in the same failure path:

Bounding an attempt means cancelling a live fastmcp session, which cancels the
SDK's anyio task group; its teardown groups that CancelledError with any child
failure. A group carrying a BaseException leaf is not an Exception, so it
escaped the poll loop as a raw anyio traceback — losing the remaining retry
budget and the caller's diagnostics pointer, which is exactly the failure mode
this helper exists to prevent. Classify groups the way
custom_components/ha_mcp_tools/llm_api.py already does: catch
BaseExceptionGroup, but retry only when EVERY leaf is transient so a real bug
inside the group still propagates. Also add the anyio stream errors
(ClosedResourceError, BrokenResourceError, EndOfStream) that a mid-read
teardown raises — plain Exceptions, not OSError.

Handing each attempt all the remaining time made the first stall monopolize the
window, so a stall was retried zero times while the docstring promised
"transient errors are retried". Cap each attempt at _ADDON_HA_LINK_PROBE_S and
retry within the overall budget.

Set fastmcp's own deadlines strictly tighter than the outer backstop. Each
starts its clock later than the one around it, so with equal values the
backstop always won the race and the phase-naming errors ("Failed to initialize
server session", the session read timeout) could never fire — every stall
degraded to a bare TimeoutError() whose repr says nothing. Name the target when
the backstop does fire, and log the first transient at INFO, since the e2e
harness runs at INFO and the cause was previously DEBUG-only: a failed link
reported no reason at all.

Correct two overstated comments: the total can overshoot by fastmcp's shielded
disconnect timeout, and only the read component of the connect phase is
budget-scoped.

Skip gates now require an explicit `run == 'false'` rather than treating any
non-`true` value as docs-only, so a future edit that lets the classifier exit 0
without writing its output cannot silently reopen the hole this PR closes.
tests/src/unit/test_e2e_skip_gate_shape.py walks every classifier-gated lane and
pins both clauses plus the pr.yml gate step.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: kingpanther13

.github/workflows/haos-e2e-embedded-tests.yml

@@ -116,7 +116,14 @@ jobs:
     needs: changes
     # A skipped job reports Success to the required status check (unlike a
     # workflow-level path-skip, which never reports). See the `changes` job.
-    if: needs.changes.outputs.run == 'true'
+    # Skipping requires a classifier that SUCCEEDED and said `false` out loud.
+    # Two ways this used to fail open: an `if:` with no status-check function
+    # implicitly ANDs with success(), so a failed classifier skipped this suite
+    # and reported Success (a runner hiccup in a 5-minute filter job waving code
+    # through untested); and an empty `run` — which is what a non-success
+    # classifier leaves behind — read the same as "docs-only". Any value other
+    # than `false` now runs. `!cancelled()` keeps a real cancellation cancelling.
+    if: ${{ !cancelled() && (needs.changes.result != 'success' || needs.changes.outputs.run != 'false') }}
     runs-on: ubuntu-22.04
     # 60 (vs the external/inaddon lanes' 45): the embedded lane pays a per-worker
     # session-setup cost the others don't — enabling the entry kicks off a runtime

.github/workflows/haos-e2e-inaddon-tests.yml

@@ -119,7 +119,14 @@ jobs:
     needs: changes
     # A skipped job reports Success to the required status check (unlike a
     # workflow-level path-skip, which never reports). See the `changes` job.
-    if: needs.changes.outputs.run == 'true'
+    # Skipping requires a classifier that SUCCEEDED and said `false` out loud.
+    # Two ways this used to fail open: an `if:` with no status-check function
+    # implicitly ANDs with success(), so a failed classifier skipped this suite
+    # and reported Success (a runner hiccup in a 5-minute filter job waving code
+    # through untested); and an empty `run` — which is what a non-success
+    # classifier leaves behind — read the same as "docs-only". Any value other
+    # than `false` now runs. `!cancelled()` keeps a real cancellation cancelling.
+    if: ${{ !cancelled() && (needs.changes.result != 'success' || needs.changes.outputs.run != 'false') }}
     runs-on: ubuntu-22.04
     timeout-minutes: 45
 

.github/workflows/haos-e2e-tests.yml

@@ -109,7 +109,14 @@ jobs:
     needs: changes
     # A skipped job reports Success to the required status check (unlike a
     # workflow-level path-skip, which never reports). See the `changes` job.
-    if: needs.changes.outputs.run == 'true'
+    # Skipping requires a classifier that SUCCEEDED and said `false` out loud.
+    # Two ways this used to fail open: an `if:` with no status-check function
+    # implicitly ANDs with success(), so a failed classifier skipped this suite
+    # and reported Success (a runner hiccup in a 5-minute filter job waving code
+    # through untested); and an empty `run` — which is what a non-success
+    # classifier leaves behind — read the same as "docs-only". Any value other
+    # than `false` now runs. `!cancelled()` keeps a real cancellation cancelling.
+    if: ${{ !cancelled() && (needs.changes.result != 'success' || needs.changes.outputs.run != 'false') }}
     runs-on: ubuntu-22.04
     timeout-minutes: 45
 

.github/workflows/pr.yml

@@ -90,7 +90,17 @@ jobs:
     steps:
       - name: Require E2E success unless skipped
         run: |
-          if [ "${{ needs.changes.outputs.run }}" != "true" ]; then
+          # Only a classifier that SUCCEEDED and said `false` out loud can
+          # authorize the skip. A non-success classifier leaves `run` empty, and
+          # passing on that would make a runner hiccup indistinguishable from a
+          # docs-only PR — the lanes above run in that case, so fall through and
+          # judge them on their results. Keep this branch equivalent to the lane
+          # `if:` predicates above; if they diverge, this gate demands success
+          # from a lane that never ran.
+          classifier_result="${{ needs.changes.result }}"
+          if [ "$classifier_result" != "success" ]; then
+            echo "::warning::change classifier did not succeed (result=$classifier_result); judging the lanes on their own results."
+          elif [ "${{ needs.changes.outputs.run }}" = "false" ]; then
             echo "Docs/website-only PR — E2E Validation skipped; gate passes."
             exit 0
           fi
@@ -104,7 +114,12 @@ jobs:
           # the update-path lane must all pass. Any one failing wedges the
           # required gate.
           if [ "$container_result" != "success" ] || [ "$embedded_result" != "success" ] || [ "$update_path_result" != "success" ]; then
-            echo "::error::E2E Validation did not succeed (container=$container_result, embedded=$embedded_result, update-path=$update_path_result)."
+            # `skipped` here is NOT the docs-only skip (that exited 0 above): the
+            # lanes never started, e.g. the run was cancelled.
+            case "$container_result$embedded_result$update_path_result" in
+              *skipped*) echo "::error::E2E Validation lanes did not start (results: container=$container_result, embedded=$embedded_result, update-path=$update_path_result) — the run may have been cancelled." ;;
+              *) echo "::error::E2E Validation did not succeed (container=$container_result, embedded=$embedded_result, update-path=$update_path_result)." ;;
+            esac
             exit 1
           fi
 
@@ -337,7 +352,11 @@ jobs:
     needs: changes
     # A skipped job reports Success to the required status check, so a
     # docs/website-only PR doesn't wedge the merge waiting on these lanes.
-    if: needs.changes.outputs.run == 'true'
+    # Skipping requires a classifier that succeeded and said `false` out loud;
+    # anything else (a failed classifier leaves `run` empty) runs this lane.
+    # Keep this predicate equivalent to the skip branch in e2e-validation-gate:
+    # if they diverge, the gate demands success from a lane that never ran.
+    if: ${{ !cancelled() && (needs.changes.result != 'success' || needs.changes.outputs.run != 'false') }}
     runs-on: ${{ matrix.os }}
     # 20 (was 15): headroom for the in-process-server e2e test that runs on
     # THIS lane (workflows/embedded/test_embedded_server.py) - its first
@@ -438,7 +457,11 @@ jobs:
     needs: changes
     # A skipped job reports Success, so a docs/website-only PR doesn't wedge the
     # gate waiting on this lane (mirrors e2e-validation).
-    if: needs.changes.outputs.run == 'true'
+    # Skipping requires a classifier that succeeded and said `false` out loud;
+    # anything else (a failed classifier leaves `run` empty) runs this lane.
+    # Keep this predicate equivalent to the skip branch in e2e-validation-gate:
+    # if they diverge, the gate demands success from a lane that never ran.
+    if: ${{ !cancelled() && (needs.changes.result != 'success' || needs.changes.outputs.run != 'false') }}
     runs-on: ${{ matrix.os }}
     timeout-minutes: 25
     strategy:
@@ -537,7 +560,11 @@ jobs:
     needs: changes
     # A skipped job reports Success, so a docs/website-only PR doesn't wedge the
     # gate waiting on this lane (mirrors e2e-validation).
-    if: needs.changes.outputs.run == 'true'
+    # Skipping requires a classifier that succeeded and said `false` out loud;
+    # anything else (a failed classifier leaves `run` empty) runs this lane.
+    # Keep this predicate equivalent to the skip branch in e2e-validation-gate:
+    # if they diverge, the gate demands success from a lane that never ran.
+    if: ${{ !cancelled() && (needs.changes.result != 'success' || needs.changes.outputs.run != 'false') }}
     runs-on: ubuntu-latest
     # 30m: each of the two scenarios (component@stable and component@working-tree)
     # does a first bring-up that installs ha-mcp plus the full fastmcp dependency

tests/src/haos_runtime.py

@@ -1379,10 +1379,14 @@ def wait_for_addon_mcp_ready(*, timeout: float = 300.0) -> str:
 # when something is genuinely wrong.
 _ADDON_HA_LINK_TIMEOUT_S = 180.0
 _ADDON_HA_LINK_POLL_S = 3.0
+# Ceiling for ONE attempt. Long enough for a healthy connect + initialize +
+# tool round trip on a loaded runner, short enough that a stalled attempt is
+# abandoned and retried inside the overall budget instead of monopolizing it.
+_ADDON_HA_LINK_PROBE_S = 30.0
 
 
-def _addon_link_transient_errors() -> tuple[type[BaseException], ...]:
-    """Return the errors that mean "not linked yet" rather than "broken".
+def _addon_link_transient_leaves() -> tuple[type[BaseException], ...]:
+    """Return the leaf errors that mean "not linked yet" rather than "broken".
 
     Mirrors the suite's canonical MCP-polling set
     (``e2e/utilities/wait_helpers._POLLING_TRANSIENT_ERRORS``) plus httpx
@@ -1393,6 +1397,7 @@ def _addon_link_transient_errors() -> tuple[type[BaseException], ...]:
     ``test_haos_addon_ha_link_wait`` pins that it stays a superset of the
     canonical tuple.
     """
+    import anyio
     import httpx
     from fastmcp.exceptions import ClientError, FastMCPError
     from mcp import McpError
@@ -1405,23 +1410,96 @@ def _addon_link_transient_errors() -> tuple[type[BaseException], ...]:
         OSError,
         TimeoutError,
         httpx.HTTPError,
+        # Cancelling a stalled attempt tears the streamable-HTTP memory
+        # streams down mid-read. These are plain Exceptions, not OSError.
+        anyio.ClosedResourceError,
+        anyio.BrokenResourceError,
+        anyio.EndOfStream,
     )
 
 
-def _probe_addon_ha_link(addon_mcp_url: str) -> None:
-    """Make one MCP call that needs Home Assistant; raise if it does not work."""
+def _addon_link_transient_errors() -> tuple[type[BaseException], ...]:
+    """Return the ``except`` target for one probe attempt.
+
+    Includes ``BaseExceptionGroup`` because bounding an attempt means
+    cancelling a live anyio task group, whose teardown groups our
+    ``CancelledError`` with any child failure — and a group carrying a
+    ``BaseException`` leaf is not an ``Exception``, so it would otherwise
+    escape the poll loop as a raw anyio traceback, losing both the retry
+    budget and the caller's diagnostics pointer. A caught group must still
+    pass :func:`_is_transient_link_error`, or a genuine bug inside the group
+    would be relabelled "not linked yet" (same rule as
+    ``custom_components/ha_mcp_tools/llm_api.py::_transport_errors``).
+    """
+    return (*_addon_link_transient_leaves(), BaseExceptionGroup)
+
+
+def _is_transient_link_error(err: BaseException) -> bool:
+    """Return True when every leaf of ``err`` is a transient link failure.
+
+    A group counts only when EVERY leaf (nested groups included) is
+    transient: a group carrying any other member is a real bug that must
+    propagate with its traceback instead of being retried until the deadline.
+    """
+    import asyncio
+
+    if isinstance(err, BaseExceptionGroup):
+        return all(_is_transient_link_error(exc) for exc in err.exceptions)
+    # CancelledError counts only as a group leaf — it is this module's own
+    # wait_for abandoning a stalled attempt. A *bare* CancelledError is not in
+    # the except target above, so an outside cancellation still propagates.
+    return isinstance(err, (*_addon_link_transient_leaves(), asyncio.CancelledError))
+
+
+def _probe_addon_ha_link(addon_mcp_url: str, budget: float) -> None:
+    """Make one MCP call that needs Home Assistant; raise if it does not work.
+
+    ``budget`` bounds this single attempt. Without it the attempt inherits
+    FastMCP's Streamable HTTP default of ``httpx.Timeout(30.0, read=300.0)``
+    (``client/transports/http.py`` — the one place this default is cited), so a
+    listener that accepts the connection but stalls on initialize or the tool
+    response holds the poll loop for up to 300s, far past the caller's own
+    deadline, delaying the fixture failure and its HAOS diagnostics.
+
+    FastMCP's own deadlines are set strictly tighter than the outer backstop.
+    Each starts its clock later than the one around it — ``init_timeout``
+    after session setup, the read timeout at request send — so with equal
+    values the backstop would always win the race and their phase-naming
+    errors ("Failed to initialize server session", the session read timeout)
+    could never fire, degrading every stall to a message-less ``TimeoutError``.
+    The raw TCP connect keeps httpx's own fixed connect timeout rather than
+    ``budget``; the outer coroutine bound is what covers a stall there.
+    """
     import asyncio
 
     from fastmcp import Client
     from fastmcp.client.transports import StreamableHttpTransport
 
+    inner = max(budget * 0.8, budget - 2.0)
+
     async def _call() -> None:
         # Fresh client per attempt: the server is stateless, and a long-lived
         # session would not survive the addon restarting mid-poll.
-        async with Client(StreamableHttpTransport(url=addon_mcp_url)) as client:
-            await client.call_tool("ha_get_addon", {})
+        async with Client(
+            StreamableHttpTransport(url=addon_mcp_url),
+            timeout=inner,
+            init_timeout=inner,
+        ) as client:
+            await client.call_tool("ha_get_addon", {}, timeout=inner)
+
+    async def _bounded() -> None:
+        try:
+            await asyncio.wait_for(_call(), timeout=budget)
+        except TimeoutError as exc:
+            # wait_for raises a bare TimeoutError(), whose repr carries nothing.
+            # Name the target so the loop's final report is actionable.
+            raise TimeoutError(
+                f"ha_get_addon at {addon_mcp_url} did not answer within "
+                f"{budget:.1f}s (fastmcp's own {inner:.1f}s deadlines did not "
+                f"fire, so the stall was not in initialize or the read)"
+            ) from exc
 
-    asyncio.run(_call())
+    asyncio.run(_bounded())
 
 
 def wait_for_addon_ha_link_ready(
@@ -1441,21 +1519,41 @@ def wait_for_addon_ha_link_ready(
     Core WebSocket and the Supervisor API) and is the call that surfaced the
     race. Returns False on timeout so the caller can fail with context.
     Transient errors are retried; bugs propagate.
+
+    Each attempt is capped by ``_ADDON_HA_LINK_PROBE_S`` and by the time left
+    on the deadline, so a single stalled attempt is abandoned and retried
+    rather than consuming the whole window. ``timeout`` is the ceiling up to
+    FastMCP's disconnect timeout: tearing down a cancelled attempt runs
+    shielded cleanup scopes (``client_disconnect_timeout``, 5s default) that
+    cancellation cannot interrupt, so the total can overshoot by a few seconds.
     """
     deadline = time.monotonic() + timeout
     transient = _addon_link_transient_errors()
     last_err: BaseException | None = None
-    while time.monotonic() < deadline:
+    attempts = 0
+    while True:
+        remaining = deadline - time.monotonic()
+        if remaining <= 0:
+            break
+        attempts += 1
         try:
-            _probe_addon_ha_link(addon_mcp_url)
+            _probe_addon_ha_link(addon_mcp_url, min(remaining, _ADDON_HA_LINK_PROBE_S))
         except transient as e:
+            if not _is_transient_link_error(e):
+                raise
             last_err = e
-            LOG.debug("Addon -> Home Assistant link not ready yet: %r", e)
+            # The e2e harness logs at INFO, so a DEBUG-only cause would leave a
+            # failed link reported with no explanation at all. First cause at
+            # INFO, the repeats at DEBUG so a slow boot stays quiet.
+            log = LOG.info if attempts == 1 else LOG.debug
+            log("Addon -> Home Assistant link not ready yet: %r", e)
         else:
             elapsed = int(time.monotonic() - (deadline - timeout))
             LOG.info("Addon -> Home Assistant link ready after ~%ds", elapsed)
             return True
-        time.sleep(_ADDON_HA_LINK_POLL_S)
+        # Never sleep past the deadline, so the loop does not add a whole
+        # extra poll interval on top of the budget.
+        time.sleep(min(_ADDON_HA_LINK_POLL_S, max(0.0, deadline - time.monotonic())))
     LOG.error(
         "Addon -> Home Assistant link never came up within %.0fs (last_exc=%r)",
         timeout,