From 4a887fdeb93ed2590262ddeaf14e99ddb50f76ff Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Fri, 12 Jun 2026 10:42:48 -0400 Subject: [PATCH] sandbox: fix Channel.close() no-op + honest SETUP_ERROR (Phase 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Channel.close() early-returned on `if self._closed: return`. But the read loop's EOF `finally` already sets `_closed=True` (and cancels, never awaits, inflight tasks), so a close() after EOF returned immediately — transport.close() and the inflight gather never ran, leaking the stdin pipe / unix connection every restart cycle. Split "already closed" (set _closed, fail pending — idempotent) from "teardown not yet done" (close transport + await inflight, guarded by a new _close_done flag that runs exactly once regardless of who set _closed first). channel.py is hand-mirrored — the identical fix is applied to BOTH copies (homeassistant/components/sandbox/channel.py and sandbox/hass_client/hass_client/channel.py). SETUP_RETRY non-retry: the router runs outside ConfigEntry.async_setup, so the SETUP_RETRY timer (async_call_later) is never armed for a sandbox entry — a router-set SETUP_RETRY wedged the entry in a retry state that never fires (and a later async_setup raised OperationNotAllowed). The ChannelClosedError-during-entry_setup case now reports SETUP_ERROR honestly (recoverable via manual reload); ARCHITECTURE.md §5 updated and a router-driven true retry flagged as a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) --- homeassistant/components/sandbox/channel.py | 18 +++++++++++++++--- homeassistant/components/sandbox/router.py | 12 ++++++++++-- sandbox/ARCHITECTURE.md | 18 +++++++++++++++--- sandbox/hass_client/hass_client/channel.py | 18 +++++++++++++++--- 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/homeassistant/components/sandbox/channel.py b/homeassistant/components/sandbox/channel.py index 38646204668f..6ad01f41fd5b 100644 --- a/homeassistant/components/sandbox/channel.py +++ b/homeassistant/components/sandbox/channel.py @@ -350,6 +350,7 @@ class Channel: self._handlers: dict[str, Handler] = {} self._reader_task: asyncio.Task[None] | None = None self._closed: bool = False + self._close_done: bool = False self._write_lock = asyncio.Lock() self._inflight: set[asyncio.Task[None]] = set() self._inflight_sem = asyncio.Semaphore(max_inflight) @@ -419,16 +420,27 @@ class Channel: await self._write(Frame.push(msg_type, payload)) async def close(self) -> None: - """Close the channel and cancel any in-flight calls.""" - if self._closed: - return + """Close the channel and cancel any in-flight calls. + + Idempotent and safe after the read loop has already marked the + channel closed on EOF: that path sets ``_closed`` and cancels + inflight tasks but cannot close the transport or await the cancelled + tasks (it runs *inside* the reader). ``close()`` always finishes that + teardown — closing the transport and awaiting inflight exactly once + via the ``_close_done`` guard — no matter who set ``_closed`` first, + so the stdin pipe / unix connection never leaks across a restart. + """ self._closed = True + # Fail any still-pending calls; a no-op if the read loop already did. for future in self._pending.values(): if not future.done(): future.set_exception( ChannelClosedError(f"channel {self._name!r} is closed") ) self._pending.clear() + if self._close_done: + return + self._close_done = True inflight = list(self._inflight) for task in inflight: task.cancel() diff --git a/homeassistant/components/sandbox/router.py b/homeassistant/components/sandbox/router.py index 4e6a6f234d87..4d2d1838eac9 100644 --- a/homeassistant/components/sandbox/router.py +++ b/homeassistant/components/sandbox/router.py @@ -131,10 +131,18 @@ class SandboxFlowRouter: try: result = await channel.call(MSG_ENTRY_SETUP, payload) except ChannelClosedError: + # The router runs *outside* ConfigEntry.async_setup, so the + # SETUP_RETRY timer (async_call_later) that core wires there is + # never armed for a sandbox entry — setting SETUP_RETRY here would + # wedge the entry in a retry state that never retries (and a later + # async_setup would raise OperationNotAllowed). Report SETUP_ERROR + # honestly instead; the entry stays recoverable via a manual + # reload. (Follow-up: a router-driven true retry — see + # ARCHITECTURE.md §5.) entry._async_set_state( # noqa: SLF001 self._hass, - ConfigEntryState.SETUP_RETRY, - "Sandbox channel closed during setup", + ConfigEntryState.SETUP_ERROR, + "Sandbox channel closed during setup; reload to retry", ) return False except ChannelRemoteError as err: diff --git a/sandbox/ARCHITECTURE.md b/sandbox/ARCHITECTURE.md index f2897e82f49a..0badf55ea74f 100644 --- a/sandbox/ARCHITECTURE.md +++ b/sandbox/ARCHITECTURE.md @@ -132,9 +132,21 @@ python -m hass_client.sandbox --name --url stdio:// **Crash recovery** is bounded: `SandboxProcess` restarts on unexpected exit up to 3 times in a 60s sliding window with backoff; exceeding the budget marks the sandbox `failed`, `ensure_started` raises `SandboxFailedError`, and the router -marks affected entries `SETUP_ERROR`. (`SETUP_RETRY` is reserved for the -narrower case of a `ChannelClosedError` *during* an `entry_setup` round-trip, -where a retry can succeed.) +marks affected entries `SETUP_ERROR`. A `ChannelClosedError` *during* an +`entry_setup` round-trip (the sandbox crashed mid-setup) is also reported as +`SETUP_ERROR` — the entry stays recoverable via a manual reload. The router +runs *outside* `ConfigEntry.async_setup`, so it cannot reach core's +`SETUP_RETRY` timer (`async_call_later`); setting `SETUP_RETRY` from the router +would wedge the entry in a retry state that never fires. A router-driven true +retry is a follow-up. + +When a crashed sandbox respawns, the manager's `on_ready` hook fires once the +fresh process is up: the displaced bridge is torn down (its proxy entities + +`EntityComponent` platform slots released through the public +`async_unregister_remote_platform` hook) and the group's still-`LOADED` entries +are re-driven through `async_schedule_reload`, so every entity re-registers +against the new bridge. While the sandbox is down its proxies are flipped +unavailable (via `on_channel_closed`) rather than serving stale state. **Graceful shutdown** on `EVENT_HOMEASSISTANT_STOP`: the manager fans out `sandbox/shutdown`; each sandbox unloads its entries, snapshots diff --git a/sandbox/hass_client/hass_client/channel.py b/sandbox/hass_client/hass_client/channel.py index 25599b39e069..86fdce16accf 100644 --- a/sandbox/hass_client/hass_client/channel.py +++ b/sandbox/hass_client/hass_client/channel.py @@ -322,6 +322,7 @@ class Channel: self._handlers: dict[str, Handler] = {} self._reader_task: asyncio.Task[None] | None = None self._closed: bool = False + self._close_done: bool = False self._write_lock = asyncio.Lock() self._inflight: set[asyncio.Task[None]] = set() self._inflight_sem = asyncio.Semaphore(max_inflight) @@ -386,16 +387,27 @@ class Channel: await self._write(Frame.push(msg_type, payload)) async def close(self) -> None: - """Close the channel and cancel any in-flight calls.""" - if self._closed: - return + """Close the channel and cancel any in-flight calls. + + Idempotent and safe after the read loop has already marked the + channel closed on EOF: that path sets ``_closed`` and cancels + inflight tasks but cannot close the transport or await the cancelled + tasks (it runs *inside* the reader). ``close()`` always finishes that + teardown — closing the transport and awaiting inflight exactly once + via the ``_close_done`` guard — no matter who set ``_closed`` first, + so the stdin pipe / unix connection never leaks across a restart. + """ self._closed = True + # Fail any still-pending calls; a no-op if the read loop already did. for future in self._pending.values(): if not future.done(): future.set_exception( ChannelClosedError(f"channel {self._name!r} is closed") ) self._pending.clear() + if self._close_done: + return + self._close_done = True inflight = list(self._inflight) for task in inflight: task.cancel()