mirror of
https://github.com/home-assistant/core.git
synced 2026-08-28 02:24:46 -05:00
sandbox: fix Channel.close() no-op + honest SETUP_ERROR (Phase 5)
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
54041a6a22
commit
4a887fdeb9
@@ -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()
|
||||
|
||||
@@ -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:
|
||||
|
||||
+15
-3
@@ -132,9 +132,21 @@ python -m hass_client.sandbox --name <group> --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
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user