mirror of
https://github.com/home-assistant/supervisor.git
synced 2026-07-02 11:25:37 +01:00
Stefan Agner
a973d22e35
* Derive App state from container state
The App.state setter mixed two responsibilities: it both mutated a
private `_state` field and dispatched side effects (WebSocket events,
issue dismissal, startup_event signaling). On top of that, an installed
but never-started app stayed in AppState.UNKNOWN forever, because the
attach() image-only fallback never fires a container state-change event
and the AppState therefore kept its constructor default. Conceptually,
ContainerState.UNKNOWN ("container does not exist") and AppState.UNKNOWN
("nothing observed yet") happened to share a name but meant different
things, which made the distinction easy to lose.
Make App.state a pure derived property. The source of truth is the last
observed ContainerState (cached on the App), plus a sticky operation-
error flag for start/stop failures that the docker event stream cannot
reflect. When no container has been observed yet, the derivation falls
back to install signals: an attached instance (image present) is
STOPPED, otherwise UNKNOWN. As a side effect, an installed-but-never-
started app now correctly reports STOPPED instead of UNKNOWN.
container_state_changed updates the cached container state and routes
all side effects through a single _emit_state_change helper that diffs
old vs new derived state. The two start/stop failure paths route
through _set_operation_error. Uninstall resets the cached signals so
the derivation naturally returns UNKNOWN.
Tests use a new tests/common.force_app_state helper that pokes the
underlying signals directly; the production class no longer carries
test-only setters.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Fix App state drive to AppState.UNKNOWN
* Unify state mutation through _update_state
Previously, state-driving signal changes were spread across two helpers
(_set_operation_error, _emit_state_change(old_state)) and required each
caller to capture self.state before mutating a private field — leaking
implementation details to call sites and raising the "why am I emitting
the old state?" question pointed out in code review.
Replace both helpers with a single _update_state(*, container_state=,
operation_error=) entry point. Callers describe what changed via
keyword arguments (None leaves a signal untouched); the helper captures
the previous state, applies the updates, recomputes the derived state
and emits side effects if anything changed.
Diff against a tracked _last_state instead of a freshly derived
"current" state, so that an out-of-band mutation between updates does
not silently shift the comparison baseline. The concrete case is
App.uninstall: instance.remove() clears the docker meta mid-flow, which
would otherwise reshape the derivation (RUNNING with no healthcheck
becomes STARTED instead of STARTUP) and suppress the STARTUP transition
that resolves the start-wait task. As a side effect, the initial
UNKNOWN -> STOPPED transition on attach is also now reliably emitted.
Switch the uninstall path to ContainerState.UNKNOWN ("we know there is
no container") rather than the constructor sentinel None.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Cache app state instead of deriving on every read
Building on the previous commit, make App.state a plain read of a
cached _state field rather than re-deriving on every property access.
The derivation moves to _derive_state(), and _update_state() is the
sole place that recomputes and assigns _state, so the value consumers
read always matches what was last emitted to listeners.
This removes the _last_state bookkeeping introduced previously: with a
single cached value there is no longer a separate "derived now" vs
"last emitted" distinction to reconcile, and out-of-band mutations
(e.g. instance.remove() clearing _meta during uninstall) can no longer
silently shift what state returns between updates.
Call _update_state() at the end of load() so the cached state settles
once attach() has run. Image-only attaches do not fire a docker event,
so without this an installed app would stay in the constructor-default
UNKNOWN until first start; this also makes the initial transition on
attach observable to listeners.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Pass operation error to _derive_state instead of storing it
The two state-driving signals were not symmetric. _container_state is
genuinely persisted state ("the last thing docker told us") that
re-derivation legitimately reads across calls. _operation_error, on the
other hand, is a momentary "force ERROR for this transition" signal; the
persistence of an error condition already lives in the cached _state.
Storing it as an instance attribute implied a sticky cross-call behavior
that no call path actually exercised: every caller either set it
explicitly right before deriving (start/stop failures, container events)
or ran argless only at load time, where no failure has occurred.
Drop the _operation_error field and pass operation_error as a parameter
to _derive_state(), defaulting to False in _update_state(). A container
observation now supersedes a prior error implicitly via the default,
which lets the container-event and uninstall call sites drop their
explicit operation_error=False.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Settle load state synchronously from current_state
The argless _update_state() settle at the end of load() raced attach()'s
container-state event. attach() fires DOCKER_CONTAINER_STATE_CHANGE via
the bus, which schedules the container_state_changed listener as a task
rather than running it inline. In the deprecated-arch early-return path
there is no await between attach() and the settle, so the listener had
not run yet: _container_state was still None and the settle derived
STOPPED (instance attached) — emitting a transient UNKNOWN->STOPPED even
for a running container before the listener corrected it. The main path
only avoided this incidentally, by having awaits (check_image,
save_persist) in between for the listener to run.
Derive the load-time state synchronously from instance.current_state()
instead of relying on the asynchronously delivered event. current_state()
returns the real container state, or UNKNOWN when only an image is
present (which derives to STOPPED), so both paths settle correctly
without racing the event.
Add a regression test that loading a running container settles to
STARTED, and mock current_state() in the state-listener test which
relies on a clean UNKNOWN baseline.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
119 lines
3.8 KiB
Python
119 lines
3.8 KiB
Python
"""Test fixup app execute start."""
|
|
|
|
from unittest.mock import patch
|
|
|
|
import pytest
|
|
|
|
from supervisor.apps.app import App
|
|
from supervisor.const import AppState
|
|
from supervisor.coresys import CoreSys
|
|
from supervisor.docker.app import DockerApp
|
|
from supervisor.exceptions import DockerError
|
|
from supervisor.resolution.const import ContextType, SuggestionType
|
|
from supervisor.resolution.data import Suggestion
|
|
from supervisor.resolution.fixups.app_execute_start import FixupAppExecuteStart
|
|
|
|
from tests.apps.test_manager import BOOT_FAIL_ISSUE
|
|
from tests.common import force_app_state
|
|
|
|
EXECUTE_START_SUGGESTION = Suggestion(
|
|
SuggestionType.EXECUTE_START, ContextType.ADDON, reference="local_ssh"
|
|
)
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"state", [AppState.STARTED, AppState.STARTUP, AppState.STOPPED]
|
|
)
|
|
@pytest.mark.usefixtures("path_extern")
|
|
async def test_fixup(coresys: CoreSys, install_app_ssh: App, state: AppState):
|
|
"""Test fixup starts app."""
|
|
force_app_state(install_app_ssh, AppState.UNKNOWN)
|
|
app_execute_start = FixupAppExecuteStart(coresys)
|
|
assert app_execute_start.auto is False
|
|
|
|
async def mock_start(*args, **kwargs):
|
|
force_app_state(install_app_ssh, state)
|
|
|
|
coresys.resolution.add_issue(
|
|
BOOT_FAIL_ISSUE,
|
|
suggestions=[SuggestionType.EXECUTE_START],
|
|
)
|
|
with (
|
|
patch.object(DockerApp, "run") as run,
|
|
patch.object(App, "_wait_for_startup", new=mock_start),
|
|
patch.object(App, "write_options"),
|
|
):
|
|
await app_execute_start()
|
|
run.assert_called_once()
|
|
|
|
assert not coresys.resolution.issues
|
|
assert not coresys.resolution.suggestions
|
|
|
|
|
|
@pytest.mark.usefixtures("path_extern")
|
|
async def test_fixup_start_error(coresys: CoreSys, install_app_ssh: App):
|
|
"""Test fixup fails on start app failure."""
|
|
force_app_state(install_app_ssh, AppState.UNKNOWN)
|
|
app_execute_start = FixupAppExecuteStart(coresys)
|
|
|
|
coresys.resolution.add_issue(
|
|
BOOT_FAIL_ISSUE,
|
|
suggestions=[SuggestionType.EXECUTE_START],
|
|
)
|
|
with (
|
|
patch.object(DockerApp, "run", side_effect=DockerError) as run,
|
|
patch.object(App, "write_options"),
|
|
):
|
|
await app_execute_start()
|
|
run.assert_called_once()
|
|
|
|
assert BOOT_FAIL_ISSUE in coresys.resolution.issues
|
|
assert EXECUTE_START_SUGGESTION in coresys.resolution.suggestions
|
|
|
|
|
|
@pytest.mark.parametrize("state", [AppState.ERROR, AppState.UNKNOWN])
|
|
@pytest.mark.usefixtures("path_extern")
|
|
async def test_fixup_wait_start_failure(
|
|
coresys: CoreSys, install_app_ssh: App, state: AppState
|
|
):
|
|
"""Test fixup fails if app does not complete startup."""
|
|
force_app_state(install_app_ssh, AppState.UNKNOWN)
|
|
app_execute_start = FixupAppExecuteStart(coresys)
|
|
|
|
async def mock_start(*args, **kwargs):
|
|
force_app_state(install_app_ssh, state)
|
|
|
|
coresys.resolution.add_issue(
|
|
BOOT_FAIL_ISSUE,
|
|
suggestions=[SuggestionType.EXECUTE_START],
|
|
)
|
|
with (
|
|
patch.object(DockerApp, "run") as run,
|
|
patch.object(App, "_wait_for_startup", new=mock_start),
|
|
patch.object(App, "write_options"),
|
|
):
|
|
await app_execute_start()
|
|
run.assert_called_once()
|
|
|
|
assert BOOT_FAIL_ISSUE in coresys.resolution.issues
|
|
assert EXECUTE_START_SUGGESTION in coresys.resolution.suggestions
|
|
|
|
|
|
async def test_fixup_no_app(coresys: CoreSys):
|
|
"""Test fixup dismisses if app is missing."""
|
|
app_execute_start = FixupAppExecuteStart(coresys)
|
|
|
|
coresys.resolution.add_issue(
|
|
BOOT_FAIL_ISSUE,
|
|
suggestions=[SuggestionType.EXECUTE_START],
|
|
)
|
|
with (
|
|
patch.object(DockerApp, "run") as run,
|
|
patch.object(App, "write_options"),
|
|
):
|
|
await app_execute_start()
|
|
run.assert_not_called()
|
|
|
|
assert not coresys.resolution.issues
|
|
assert not coresys.resolution.suggestions
|