From f3788602a632b5edc296aa501b303b111774e4ad Mon Sep 17 00:00:00 2001 From: B661LP Date: Wed, 9 Sep 2026 15:01:07 +0200 Subject: [PATCH] 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 --- Makefile | 10 ++-- strix/config/settings.py | 9 +++- strix/runtime/backends.py | 59 ++++++++++++++++++++---- tests/test_podman.py | 97 +++++++++++++++++++++++++++++++++++++-- 4 files changed, 157 insertions(+), 18 deletions(-) diff --git a/Makefile b/Makefile index 3dcc58bac..d2522f086 100644 --- a/Makefile +++ b/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: diff --git a/strix/config/settings.py b/strix/config/settings.py index f522c52ea..2e4ee4fd2 100644 --- a/strix/config/settings.py +++ b/strix/config/settings.py @@ -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 diff --git a/strix/runtime/backends.py b/strix/runtime/backends.py index 085eae166..d520c1daa 100644 --- a/strix/runtime/backends.py +++ b/strix/runtime/backends.py @@ -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() diff --git a/tests/test_podman.py b/tests/test_podman.py index 708578b72..2fa8bbf5d 100644 --- a/tests/test_podman.py +++ b/tests/test_podman.py @@ -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"