mirror of
https://github.com/usestrix/strix.git
synced 2026-10-02 02:13:43 +00:00
fix(runtime): address podman fallback/normalization review feedback
- get_runtime_client no longer silently falls back to docker.from_env() for a non-docker backend when the resolved socket is unreachable or no socket resolves at all; it now raises a clear RuntimeError instead of running the sandbox on the wrong runtime - auto_detect_podman_socket now verifies each candidate responds to a ping and skips stale/dead sockets in favor of the next live candidate, instead of returning the first path that merely exists on disk - RuntimeSettings normalizes STRIX_RUNTIME_BACKEND (strip+lowercase) once, so a value like 'Podman' doesn't pass install/connection checks but then fail backend registry lookup during session creation - rename the Makefile's generic 'test' target to 'test-podman' since it only ever ran tests/test_podman.py, not the full suite
This commit is contained in:
parent
ad0a371d80
commit
f3788602a6
4 changed files with 157 additions and 18 deletions
10
Makefile
10
Makefile
|
|
@ -1,4 +1,4 @@
|
|||
.PHONY: help install dev-install format lint type-check security test check-all clean pre-commit setup-dev dev viewer wheel tui-build tui-test tui-lint
|
||||
.PHONY: help install dev-install format lint type-check security test-podman check-all clean pre-commit setup-dev dev viewer wheel tui-build tui-test tui-lint
|
||||
|
||||
TUI_BINARY := build/sidecar/strix-tui$(if $(filter Windows_NT,$(OS)),.exe)
|
||||
|
||||
|
|
@ -13,7 +13,7 @@ help:
|
|||
@echo " lint - Lint code with ruff"
|
||||
@echo " type-check - Run type checking with mypy and pyright"
|
||||
@echo " security - Run security checks with bandit"
|
||||
@echo " test - Run unit tests with pytest"
|
||||
@echo " test-podman - Run the Podman backend unit tests with pytest"
|
||||
@echo " check-all - Run all code quality checks"
|
||||
@echo ""
|
||||
@echo "Development:"
|
||||
|
|
@ -58,12 +58,12 @@ security:
|
|||
uv run bandit -r strix/ -c pyproject.toml
|
||||
@echo "✅ Security checks complete!"
|
||||
|
||||
test:
|
||||
@echo "🧪 Running unit tests with pytest..."
|
||||
test-podman:
|
||||
@echo "🧪 Running Podman backend unit tests with pytest..."
|
||||
uv run pytest tests/test_podman.py -v
|
||||
@echo "✅ Tests complete!"
|
||||
|
||||
check-all: format lint type-check security test
|
||||
check-all: format lint type-check security test-podman
|
||||
@echo "✅ All code quality checks passed!"
|
||||
|
||||
pre-commit:
|
||||
|
|
|
|||
|
|
@ -4,7 +4,7 @@ from __future__ import annotations
|
|||
|
||||
from typing import Literal
|
||||
|
||||
from pydantic import AliasChoices, Field
|
||||
from pydantic import AliasChoices, Field, field_validator
|
||||
from pydantic_settings import BaseSettings, SettingsConfigDict
|
||||
|
||||
|
||||
|
|
@ -114,6 +114,13 @@ class RuntimeSettings(BaseSettings):
|
|||
# Max screenshot/image tool outputs kept live per agent context (0 = none).
|
||||
max_context_images: int = Field(default=3, ge=0, alias="STRIX_MAX_CONTEXT_IMAGES")
|
||||
|
||||
@field_validator("backend", mode="after")
|
||||
@classmethod
|
||||
def _normalize_backend(cls, value: str) -> str:
|
||||
# Normalize once here so every consumer (registry lookup, install
|
||||
# checks, socket detection) agrees on the same casing.
|
||||
return value.strip().lower() or "docker"
|
||||
|
||||
|
||||
class TelemetrySettings(BaseSettings):
|
||||
model_config = _BASE_CONFIG
|
||||
|
|
|
|||
|
|
@ -214,16 +214,40 @@ def get_podman_socket_candidates(
|
|||
return result
|
||||
|
||||
|
||||
def _socket_is_live(socket_url: str) -> bool:
|
||||
"""Return True if a docker-compatible client can connect to and ping ``socket_url``."""
|
||||
import docker
|
||||
|
||||
try:
|
||||
client: Any = docker.DockerClient(base_url=socket_url)
|
||||
try:
|
||||
client.ping()
|
||||
finally:
|
||||
with contextlib.suppress(Exception):
|
||||
client.close()
|
||||
except Exception: # noqa: BLE001
|
||||
return False
|
||||
return True
|
||||
|
||||
|
||||
def auto_detect_podman_socket() -> str | None:
|
||||
"""Look for an existing Podman socket on the host."""
|
||||
"""Look for a live Podman socket on the host.
|
||||
|
||||
Candidates are tried in order; a candidate that exists but does not
|
||||
respond to a ping (stale socket file, wrong machine, etc.) is skipped
|
||||
in favor of the next one instead of being returned as-is.
|
||||
"""
|
||||
try:
|
||||
candidates = get_podman_socket_candidates()
|
||||
for candidate in candidates:
|
||||
try:
|
||||
if candidate.exists() or candidate.is_socket():
|
||||
return f"unix://{candidate.resolve()}"
|
||||
if not (candidate.exists() or candidate.is_socket()):
|
||||
continue
|
||||
except OSError:
|
||||
continue
|
||||
url = f"unix://{candidate.resolve()}"
|
||||
if _socket_is_live(url):
|
||||
return url
|
||||
except Exception: # noqa: BLE001
|
||||
logger.debug("Podman socket auto-detection failed", exc_info=True)
|
||||
return None
|
||||
|
|
@ -302,27 +326,44 @@ def get_runtime_client(backend: str = "docker") -> Any:
|
|||
"""Create a container runtime client for ``backend`` using multi-layer socket fallthrough:
|
||||
|
||||
STRIX_RUNTIME_SOCKET → DOCKER_HOST → per-backend auto-detection → docker.from_env() default.
|
||||
Gracefully falls through to docker.from_env() on connection/ping failure.
|
||||
|
||||
The ``docker.from_env()`` default is only used for the ``docker`` backend itself: falling
|
||||
back to it for a non-docker backend (e.g. ``podman``) would silently run the sandbox on the
|
||||
wrong runtime, so that case raises instead.
|
||||
"""
|
||||
import docker
|
||||
|
||||
socket_url = resolve_runtime_socket(backend)
|
||||
normalized_backend = (backend or "docker").strip().lower()
|
||||
socket_url = resolve_runtime_socket(normalized_backend)
|
||||
if socket_url:
|
||||
try:
|
||||
client: Any = docker.DockerClient(base_url=socket_url)
|
||||
client.ping()
|
||||
except Exception: # noqa: BLE001
|
||||
except Exception as exc:
|
||||
if normalized_backend != "docker":
|
||||
raise RuntimeError(
|
||||
f"Could not connect to the {normalized_backend} runtime via socket "
|
||||
f"{socket_url}. Set STRIX_RUNTIME_SOCKET to a reachable {normalized_backend} "
|
||||
"socket, or check that the daemon is running."
|
||||
) from exc
|
||||
logger.warning(
|
||||
"Failed to connect to %s via socket %s; falling through to default",
|
||||
backend,
|
||||
normalized_backend,
|
||||
socket_url,
|
||||
exc_info=True,
|
||||
)
|
||||
else:
|
||||
logger.info("Connected to %s runtime via socket: %s", backend, socket_url)
|
||||
logger.info("Connected to %s runtime via socket: %s", normalized_backend, socket_url)
|
||||
return client
|
||||
|
||||
logger.debug("Using docker.from_env() default for backend %s", backend)
|
||||
if normalized_backend != "docker":
|
||||
raise RuntimeError(
|
||||
f"No reachable {normalized_backend} socket found (checked STRIX_RUNTIME_SOCKET, "
|
||||
"DOCKER_HOST, and auto-detection). Set STRIX_RUNTIME_SOCKET to the "
|
||||
f"{normalized_backend} socket path, or set STRIX_RUNTIME_BACKEND=docker to use Docker."
|
||||
)
|
||||
|
||||
logger.debug("Using docker.from_env() default for backend %s", normalized_backend)
|
||||
return docker.from_env()
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ from unittest.mock import MagicMock, patch
|
|||
|
||||
import pytest
|
||||
|
||||
from strix.config.settings import RuntimeSettings
|
||||
from strix.interface.environment import check_runtime_installed
|
||||
from strix.interface.utils import check_runtime_connection
|
||||
from strix.runtime.backends import (
|
||||
|
|
@ -268,7 +269,7 @@ def test_socket_fallthrough_docker_host(monkeypatch: pytest.MonkeyPatch) -> None
|
|||
|
||||
|
||||
def test_socket_fallthrough_autodetect_podman(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Auto-detects first existing socket candidate when env vars are unset."""
|
||||
"""Auto-detects first existing, live socket candidate when env vars are unset."""
|
||||
monkeypatch.delenv("STRIX_RUNTIME_SOCKET", raising=False)
|
||||
monkeypatch.delenv("DOCKER_HOST", raising=False)
|
||||
|
||||
|
|
@ -280,15 +281,42 @@ def test_socket_fallthrough_autodetect_podman(monkeypatch: pytest.MonkeyPatch) -
|
|||
),
|
||||
patch.object(Path, "exists", return_value=True),
|
||||
patch.object(Path, "resolve", return_value=fake_sock),
|
||||
patch("strix.runtime.backends._socket_is_live", return_value=True),
|
||||
):
|
||||
detected = auto_detect_podman_socket()
|
||||
assert detected == f"unix://{fake_sock}"
|
||||
|
||||
|
||||
def test_socket_fallthrough_autodetect_podman_skips_stale_candidate(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""A stale/unreachable candidate is skipped in favor of the next live one."""
|
||||
monkeypatch.delenv("STRIX_RUNTIME_SOCKET", raising=False)
|
||||
monkeypatch.delenv("DOCKER_HOST", raising=False)
|
||||
|
||||
stale_sock = Path("/run/user/1000/podman/podman.sock")
|
||||
live_sock = Path("/run/podman/podman.sock")
|
||||
|
||||
def fake_is_live(url: str) -> bool:
|
||||
return url == f"unix://{live_sock}"
|
||||
|
||||
with (
|
||||
patch(
|
||||
"strix.runtime.backends.get_podman_socket_candidates",
|
||||
return_value=[stale_sock, live_sock],
|
||||
),
|
||||
patch.object(Path, "exists", return_value=True),
|
||||
patch.object(Path, "resolve", side_effect=[stale_sock, live_sock]),
|
||||
patch("strix.runtime.backends._socket_is_live", side_effect=fake_is_live),
|
||||
):
|
||||
detected = auto_detect_podman_socket()
|
||||
assert detected == f"unix://{live_sock}"
|
||||
|
||||
|
||||
def test_socket_fallthrough_graceful_on_missing_or_failed_socket(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""Connection failure on detected socket gracefully falls through to docker.from_env()."""
|
||||
"""Docker backend connection failure gracefully falls through to docker.from_env()."""
|
||||
monkeypatch.delenv("STRIX_RUNTIME_SOCKET", raising=False)
|
||||
monkeypatch.delenv("DOCKER_HOST", raising=False)
|
||||
|
||||
|
|
@ -307,12 +335,55 @@ def test_socket_fallthrough_graceful_on_missing_or_failed_socket(
|
|||
return_value="unix:///unreachable.sock",
|
||||
),
|
||||
):
|
||||
client = get_runtime_client("podman")
|
||||
client = get_runtime_client("docker")
|
||||
|
||||
assert client is mock_default_client
|
||||
mock_docker.from_env.assert_called_once()
|
||||
|
||||
|
||||
def test_get_runtime_client_podman_raises_instead_of_falling_back_to_docker(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""A failed Podman socket connection raises rather than silently using Docker."""
|
||||
monkeypatch.delenv("STRIX_RUNTIME_SOCKET", raising=False)
|
||||
monkeypatch.delenv("DOCKER_HOST", raising=False)
|
||||
|
||||
mock_docker = MagicMock()
|
||||
mock_bad_client = MagicMock()
|
||||
mock_bad_client.ping.side_effect = ConnectionRefusedError("Daemon unreachable")
|
||||
mock_docker.DockerClient.return_value = mock_bad_client
|
||||
|
||||
with (
|
||||
patch.dict("sys.modules", {"docker": mock_docker}),
|
||||
patch(
|
||||
"strix.runtime.backends.resolve_runtime_socket",
|
||||
return_value="unix:///unreachable.sock",
|
||||
),
|
||||
pytest.raises(RuntimeError, match="podman"),
|
||||
):
|
||||
get_runtime_client("podman")
|
||||
|
||||
mock_docker.from_env.assert_not_called()
|
||||
|
||||
|
||||
def test_get_runtime_client_podman_raises_when_no_socket_resolved(
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""No resolvable Podman socket at all raises rather than defaulting to Docker."""
|
||||
monkeypatch.delenv("STRIX_RUNTIME_SOCKET", raising=False)
|
||||
monkeypatch.delenv("DOCKER_HOST", raising=False)
|
||||
|
||||
mock_docker = MagicMock()
|
||||
with (
|
||||
patch.dict("sys.modules", {"docker": mock_docker}),
|
||||
patch("strix.runtime.backends.resolve_runtime_socket", return_value=None),
|
||||
pytest.raises(RuntimeError, match="podman"),
|
||||
):
|
||||
get_runtime_client("podman")
|
||||
|
||||
mock_docker.from_env.assert_not_called()
|
||||
|
||||
|
||||
def test_socket_fallthrough_strix_runtime_socket_raw_path_normalization() -> None:
|
||||
"""Raw socket path is normalized to unix:// URI scheme."""
|
||||
assert normalize_socket_url("/run/podman/podman.sock") == "unix:///run/podman/podman.sock"
|
||||
|
|
@ -377,3 +448,23 @@ def test_check_runtime_connection_podman_uses_podman_backend(
|
|||
mock_get.assert_called_once_with("podman")
|
||||
mock_client.ping.assert_called_once()
|
||||
assert client is mock_client
|
||||
|
||||
|
||||
# ============================================================================
|
||||
# 8. Runtime Backend Setting Normalization (2 tests)
|
||||
# ============================================================================
|
||||
|
||||
|
||||
def test_runtime_settings_backend_is_lowercased(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""A mixed-case STRIX_RUNTIME_BACKEND is normalized so get_backend() finds it."""
|
||||
monkeypatch.setenv("STRIX_RUNTIME_BACKEND", "Podman")
|
||||
settings = RuntimeSettings()
|
||||
assert settings.backend == "podman"
|
||||
assert get_backend(settings.backend) is not None
|
||||
|
||||
|
||||
def test_runtime_settings_backend_defaults_when_blank(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""A blank/whitespace-only STRIX_RUNTIME_BACKEND still normalizes to the docker default."""
|
||||
monkeypatch.setenv("STRIX_RUNTIME_BACKEND", " ")
|
||||
settings = RuntimeSettings()
|
||||
assert settings.backend == "docker"
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue