mirror of
https://github.com/home-assistant/supervisor.git
synced 2026-02-14 23:19:37 +00:00
Fix Docker exec exit code handling by using detach=False (#6520)
* Fix Docker exec exit code handling by using detach=False When executing commands inside containers using `container_run_inside()`, the exec metadata did not contain a valid exit code because `detach=True` starts the exec in the background and returns immediately before completion. Root cause: With `detach=True`, Docker's exec start() returns an awaitable that yields output bytes. However, the await only waits for the HTTP/REST call to complete, NOT for the actual exec command to finish. The command continues running in the background after the HTTP response is received. Calling `inspect()` immediately after returns `ExitCode: None` because the exec hasn't completed yet. Solution: Use `detach=False` which returns a Stream object that: - Automatically waits for exec completion by reading from the stream - Provides actual command output (not just empty bytes) - Makes exit code immediately available after stream closes - No polling needed Changes: - Switch from `detach=True` to `detach=False` in container_run_inside() - Read output from stream using async context manager - Add defensive validation to ensure ExitCode is never None - Update tests to mock the Stream interface using AsyncMock - Add debug log showing exit code after command execution Fixes #6518 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Address review feedback --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -61,6 +61,7 @@ from ..const import (
|
||||
from ..coresys import CoreSys
|
||||
from ..docker.addon import DockerAddon
|
||||
from ..docker.const import ContainerState
|
||||
from ..docker.manager import ExecReturn
|
||||
from ..docker.monitor import DockerContainerStateEvent
|
||||
from ..docker.stats import DockerStats
|
||||
from ..exceptions import (
|
||||
@@ -1227,10 +1228,11 @@ class Addon(AddonModel):
|
||||
|
||||
async def _backup_command(self, command: str) -> None:
|
||||
try:
|
||||
command_return = await self.instance.run_inside(command)
|
||||
command_return: ExecReturn = await self.instance.run_inside(command)
|
||||
if command_return.exit_code != 0:
|
||||
_LOGGER.debug(
|
||||
"Pre-/Post backup command failed with: %s", command_return.output
|
||||
"Pre-/Post backup command failed with: %s",
|
||||
command_return.output.decode("utf-8", errors="replace"),
|
||||
)
|
||||
raise AddonPrePostBackupCommandReturnedError(
|
||||
_LOGGER.error, addon=self.slug, exit_code=command_return.exit_code
|
||||
|
||||
@@ -18,6 +18,7 @@ from typing import Any, Final, Self, cast
|
||||
import aiodocker
|
||||
from aiodocker.containers import DockerContainer, DockerContainers
|
||||
from aiodocker.images import DockerImages
|
||||
from aiodocker.stream import Stream
|
||||
from aiodocker.types import JSONObject
|
||||
from aiohttp import ClientTimeout, UnixConnector
|
||||
import attr
|
||||
@@ -967,17 +968,41 @@ class DockerAPI(CoreSysAttributes):
|
||||
f"Can't get container {name} to run command: {err!s}"
|
||||
) from err
|
||||
|
||||
# Execute
|
||||
# Execute - use detach=False to wait for completion and capture output
|
||||
try:
|
||||
docker_exec = await docker_container.exec(command)
|
||||
output = await docker_exec.start(detach=True)
|
||||
# start() with detach=False returns a Stream object (not a coroutine)
|
||||
stream: Stream = docker_exec.start(detach=False)
|
||||
|
||||
# Read all output from the stream until exec completes
|
||||
output_parts = []
|
||||
async with stream:
|
||||
while msg := await stream.read_out():
|
||||
output_parts.append(msg.data)
|
||||
|
||||
# Combine all output parts
|
||||
output = b"".join(output_parts)
|
||||
|
||||
# After stream closes, exec is complete and exit code should be available
|
||||
exec_metadata = await docker_exec.inspect()
|
||||
|
||||
# Validate exit code is present
|
||||
exit_code = exec_metadata.get("ExitCode")
|
||||
if exit_code is None:
|
||||
raise DockerError(
|
||||
f"Exec command '{command}' in {name} completed but ExitCode is None. "
|
||||
f"Metadata: {exec_metadata}",
|
||||
_LOGGER.error,
|
||||
)
|
||||
except aiodocker.DockerError as err:
|
||||
raise DockerError(
|
||||
f"Can't run command in container {name}: {err!s}"
|
||||
) from err
|
||||
|
||||
return ExecReturn(exec_metadata["ExitCode"], output)
|
||||
_LOGGER.debug(
|
||||
"Exec command '%s' in %s exited with %d", command, name, exit_code
|
||||
)
|
||||
return ExecReturn(exit_code, output)
|
||||
|
||||
async def remove_image(
|
||||
self, image: str, version: AwesomeVersion, latest: bool = True
|
||||
|
||||
@@ -195,7 +195,9 @@ async def docker() -> DockerAPI:
|
||||
docker_container.log = AsyncMock(return_value=[])
|
||||
|
||||
docker_container.exec.return_value = docker_exec = MagicMock(spec=Exec)
|
||||
docker_exec.start = AsyncMock(return_value=b"")
|
||||
# start() with detach=False returns a Stream (not async)
|
||||
# Use return_value instead of side_effect to avoid it being replaced by tests
|
||||
docker_exec.start.return_value = create_mock_exec_stream(output=b"")
|
||||
docker_exec.inspect.return_value = {"ExitCode": 0}
|
||||
|
||||
docker_obj.info.logging = "journald"
|
||||
@@ -872,10 +874,53 @@ async def os_available(request: pytest.FixtureRequest) -> None:
|
||||
yield
|
||||
|
||||
|
||||
def create_mock_exec_stream(output: bytes = b"") -> AsyncMock:
|
||||
"""Create a mock stream for exec with detach=False."""
|
||||
stream = AsyncMock()
|
||||
# Set up async context manager
|
||||
stream.__aenter__.return_value = stream
|
||||
stream.__aexit__.return_value = None
|
||||
# Set up read_out to return messages then None (EOF)
|
||||
if output:
|
||||
Message = type("Message", (), {"data": output})
|
||||
stream.read_out.side_effect = [Message(), None]
|
||||
else:
|
||||
stream.read_out.return_value = None
|
||||
return stream
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
async def container(docker: DockerAPI) -> DockerContainer:
|
||||
"""Mock attrs and status for container on attach."""
|
||||
yield docker.containers.get.return_value
|
||||
container_mock = docker.containers.get.return_value
|
||||
|
||||
# Set up exec mock to return a mock stream
|
||||
# Note: This must be a regular function, not async, to match aiodocker's start() behavior
|
||||
def mock_exec_start(detach=False):
|
||||
if detach:
|
||||
# Old behavior for detach=True (shouldn't be used anymore)
|
||||
# Would need to return an awaitable
|
||||
async def _async_start():
|
||||
return b""
|
||||
|
||||
return _async_start()
|
||||
# Return mock stream for detach=False (synchronous)
|
||||
return create_mock_exec_stream(output=b"")
|
||||
|
||||
# Replace the mock's start method with a MagicMock (not AsyncMock)
|
||||
start_mock = MagicMock(side_effect=mock_exec_start)
|
||||
container_mock.exec.return_value.start = start_mock
|
||||
|
||||
# Store the original side_effect so tests can override it but we can restore if needed
|
||||
start_mock._original_side_effect = mock_exec_start
|
||||
|
||||
container_mock.exec.return_value.inspect.return_value = {
|
||||
"Running": False,
|
||||
"ExitCode": 0,
|
||||
"Pid": 12345,
|
||||
}
|
||||
|
||||
yield container_mock
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
|
||||
Reference in New Issue
Block a user